Page MenuHomeFreeBSD

emulators/sdltrs
Needs ReviewPublic

Authored by sbruno on Aug 5 2020, 6:27 PM.

Details

Reviewers
swills
lwhsu
Summary

Tandy TRS-80 I, III, IV emulator with SDL2 support.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 33091
Build 30463: arc lint + arc unit

Event Timeline

sbruno requested review of this revision.Aug 5 2020, 6:27 PM
lwhsu requested changes to this revision.Aug 6 2020, 4:22 AM
lwhsu added a subscriber: lwhsu.

I got this in poudriere:

=======================<phase: configure      >============================
===>   sdltrs-1.2.11 depends on file: /usr/local/bin/cmake - found
===>   sdltrs-1.2.11 depends on executable: ninja - found
===>   sdltrs-1.2.11 depends on file: /usr/local/bin/sdl-config - found
===>   sdltrs-1.2.11 depends on shared library: libSDL.so - found (/usr/local/lib/libSDL.so)
===>   sdltrs-1.2.11 depends on shared library: libreadline.so.8 - found (/usr/local/lib/libreadline.so.8)
===>  Configuring for sdltrs-1.2.11
===>  Performing out-of-source build
/bin/mkdir -p /wrkdirs/usr/ports/emulators/sdltrs/work/.build
-- The C compiler identification is Clang 8.0.1
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc - works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check if the system is big endian
-- Searching 16 bit integer
-- Looking for sys/types.h
-- Looking for sys/types.h - found
-- Looking for stdint.h
-- Looking for stdint.h - found
-- Looking for stddef.h
-- Looking for stddef.h - found
-- Check size of unsigned short
-- Check size of unsigned short - done
-- Searching 16 bit integer - Using unsigned short
-- Check if the system is big endian - little endian
CMake Error at CMakeLists.txt:99 (message):
  sdl2-config NOT FOUND


-- Configuring incomplete, errors occurred!
See also "/wrkdirs/usr/ports/emulators/sdltrs/work/.build/CMakeFiles/CMakeOutput.log".
*** Error code 1

Stop.
make: stopped in /usr/ports/emulators/sdltrs
emulators/sdltrs/pkg-descr
3

This line exceeds 80 characters, maybe we can use 2 spaces per line to indent?

emulators/sdltrs/pkg-plist
9

Unnecessary empty line

This revision now requires changes to proceed.Aug 6 2020, 4:22 AM
emulators/sdltrs/Makefile
21

Changing to sdl2 can fix the build.

Add pkg-message to indicate where default ROMs should go.
Add HTML documentation files.
Fix bugs with last revision (SDL version, pkg-desc line len,
empty lines).
Change default behavior to *not* exec sdltrs from a terminal.

sbruno added inline comments.
emulators/sdltrs/pkg-plist
4

Huh, this is wrong. How do I change the port to put the man page in man/man1/sdltrs.1.gz ?

emulators/sdltrs/Makefile
24–26

This is static and should be made as a regular patch.

emulators/sdltrs/pkg-plist
4

You either patch the port's build system, or you do a ${MV} in a post-install target.

Use patch files to deal with man page locations.
Update pkg-plist for change to man page location.
Use patch files to deal with changing the desktop configuration file.

Ah, sorry, I should have added that patch files need to have a small comment at the top explaining they why of what they are doing, for example, like https://reviews.freebsd.org/source/ports/browse/head/lang/perl5.32/files/patch-Makefile.SH.

In D25964#576346, @mat wrote:

Ah, sorry, I should have added that patch files need to have a small comment at the top explaining they why of what they are doing, for example, like https://reviews.freebsd.org/source/ports/browse/head/lang/perl5.32/files/patch-Makefile.SH.

ooo, good idea. thanks.

Drop all patches as they have been accepted upstream or worked around.

Bump to 1.2.12.

The author maintains actual factual releases in Gitlab. I didn't see any equivalent to GH_TAGNAME to support this, so I only used the hash of the release branch.

https://gitlab.com/jengun/sdltrs/-/releases

The port is missing a DOCS option, also, can you regenerate the pkg-plist file with make makeplist? It is strangely ordered.

In D25964#581321, @mat wrote:

The port is missing a DOCS option, also, can you regenerate the pkg-plist file with make makeplist? It is strangely ordered.

I'll regenerate the plist in a sec. With regards to the DOCS option, the CMakeLists.txt has the following, which I think means I can't chose to NOT install docs, right?

install(TARGETS sdltrs          DESTINATION ${CMAKE_INSTALL_BINDIR}/)
install(FILES src/sdltrs.1      DESTINATION ${CMAKE_INSTALL_MANDIR}/man1/)
install(FILES LICENSE           DESTINATION ${CMAKE_INSTALL_DOCDIR}/)
install(DIRECTORY html          DESTINATION ${CMAKE_INSTALL_DOCDIR}/)
install(DIRECTORY diskimages/   DESTINATION ${CMAKE_INSTALL_DATADIR}/sdltrs/)
install(FILES sdltrs.desktop    DESTINATION ${CMAKE_INSTALL_DATADIR}/applications/)
install(FILES icons/sdltrs.png  DESTINATION ${CMAKE_INSTALL_DATADIR}/icons/hicolor/48x48/apps/)
install(FILES icons/sdltrs.svg  DESTINATION ${CMAKE_INSTALL_DATADIR}/icons/hicolor/scalable/apps/)
install(FILES icons/sdltrs.xpm  DESTINATION ${CMAKE_INSTALL_DATADIR}/pixmaps/)
In D25964#581321, @mat wrote:

The port is missing a DOCS option, also, can you regenerate the pkg-plist file with make makeplist? It is strangely ordered.

I'll regenerate the plist in a sec. With regards to the DOCS option, the CMakeLists.txt has the following, which I think means I can't chose to NOT install docs, right?

install(TARGETS sdltrs          DESTINATION ${CMAKE_INSTALL_BINDIR}/)
install(FILES src/sdltrs.1      DESTINATION ${CMAKE_INSTALL_MANDIR}/man1/)
install(FILES LICENSE           DESTINATION ${CMAKE_INSTALL_DOCDIR}/)
install(DIRECTORY html          DESTINATION ${CMAKE_INSTALL_DOCDIR}/)
install(DIRECTORY diskimages/   DESTINATION ${CMAKE_INSTALL_DATADIR}/sdltrs/)
install(FILES sdltrs.desktop    DESTINATION ${CMAKE_INSTALL_DATADIR}/applications/)
install(FILES icons/sdltrs.png  DESTINATION ${CMAKE_INSTALL_DATADIR}/icons/hicolor/48x48/apps/)
install(FILES icons/sdltrs.svg  DESTINATION ${CMAKE_INSTALL_DATADIR}/icons/hicolor/scalable/apps/)
install(FILES icons/sdltrs.xpm  DESTINATION ${CMAKE_INSTALL_DATADIR}/pixmaps/)

I have no idea, I do not speak cmake at all.

If a port installs docs, it must have a DOCS option that prevent adding the docs to the package if the user prefers not to have docs. Wether the docs are always installed in the staging directory or not is a bit irrelevant, it is merely an optimisation to not install them when they are not packaged.