Add --use-port option (Contrib ports part 1)

[ypujante]

@sbc100 as you asked yesterday I have split the PR in multiple smaller ones with minimal changes in each one.

This one is introducing PORTS as a new settings and make all currently "official" ports available via this new settings.

• I have updated the settings.js file to reference the new setting as an "Alternate syntax" which triggers a documentation update.
• I have changed the show() method for each port to also reflect this change.

# emcc --show-ports
Available ports:
    ogg (-sUSE_OGG=1 or -sPORTS=ogg; zlib license)
    SDL2_gfx (-sUSE_SDL_GFX=2 or -sPORTS=sdl2_gfx; zlib license)
    SDL2_image (-sUSE_SDL_IMAGE=2 or -sPORTS=sdl2_image; zlib license)
    giflib (-sUSE_GIFLIB=1 or -sPORTS=giflib; MIT license)
    SDL2_net (-sUSE_SDL_NET=2 or -sPORTS=sdl2_net; zlib license)
    icu (-sUSE_ICU=1 or -sPORTS=icu; Unicode License)
    SDL2_mixer (-sUSE_SDL_MIXER=2 or -sPORTS=sdl2_mixer; zlib license)
    libjpeg (-sUSE_LIBJPEG=1 or -sPORTS=libjpeg; BSD license)
    vorbis (-sUSE_VORBIS or -sPORTS=vorbis; zlib license)
    SDL2 (-sUSE_SDL=2 or -sPORTS=sdl2; zlib license)
    bzip2 (-sUSE_BZIP2=1 or -sPORTS=bzip2; BSD license)
    regal (-sUSE_REGAL=1 or -sPORTS=regal; Regal license)
    harfbuzz (-sUSE_HARFBUZZ=1 or -sPORTS=harfbuzz; MIT license)
    zlib (-sUSE_ZLIB=1 or -sPORTS=zlib; zlib license)
    SDL2_ttf (-sUSE_SDL_TTF=2 or -sPORTS=sdl2_ttf; zlib license)
    libmodplug (-sUSE_MODPLUG=1 or -sPORTS=libmodplug; public domain)
    freetype (-sUSE_FREETYPE=1 or -sPORTS=freetype; freetype license)
    cocos2d (-sUSE_COCOS2D=3 or -sPORTS=cocos2d)
    sqlite (-sUSE_SQLITE3=1 or -sPORTS=sqlite3); public domain)
    Boost headers v1.70.0 (-sUSE_BOOST_HEADERS=1 or -sPORTS=boost_headers; Boost license)
    libpng (-sUSE_LIBPNG or -sPORTS=libpng; zlib license)
    bullet (-sUSE_BULLET=1 or -sPORTS=bullet; zlib license)
    mpg123 (-sUSE_MPG123=1 or -sPORTS=mpg123; zlib license)

• I have added a call in emcc.py to check that the value provided in settings.PORT correspond to a valid port
• I have added tests of the new syntax in test_other.py by simply running some of the tests with the new syntax
• In addition I fixed a bug in __init__.py/read_ports method as the check for unknown dependencies was only checking the last port loaded...

I am not sure if this what you had in mind especially the change in documentation which I can revert/modify if you don't want this.

[ypujante]

I can see that test-other is passing and it contains some of the tests I added. For example:

  test_sdl2_gfx_linkable (test_other.other) ... ok (0.000s)
  test_sdl2_linkable (test_other.other) ... ok (0.000s)
  test_sdl2_ttf (test_other.other) ... ok (0.000s)
  test_vorbis (test_other.other) ... ok (20.15s)

[sbc100]

I do wonder about the new syntax a little here.

What do we want -sPORTS=foo -sPORTS=bar to mean? As of today the latter will completely replace the former. I do have a PR out that changes all lists to be additive but that is currently blocked on some feeback: #19938

In order to avoid that problem we could just use the singular form -sUSE_PORT=foo -sUSE_PORT=bar. I think I might prefer that as its obvious to the reader what is happening. On the other had I'm not sure we have support for such additive setting that work like that either. Another reason I like this is that I mimics the -lfoo , -lbar arguments which are additive.

An alliterative would be to bypass the setting mechanism and go with --use-port but that doesn't sounds good either. What about have -lfoo look for ports automatically? Maybe thats too magic?

@kripken WDYT, should we land #19938, but without effecting any existing settings (at least not at first) and use for this new setting?

[ypujante]

@sbc100 It's interesting that you are talking about the syntax, because I was thinking about this too. I feel like accepting the syntax -sPORTS=foo for a single port is weird and would make more sense if you had to write -sPORTS=[foo]. I personally am much more in favor of -sPORT=foo -sPORT=bar when you have multiple ports, since right now you have to write -sUSE_SDL=2 -sUSE_VORBIS.

I do like (even prefer) the --port or --use-port syntax as well as long as it is additive.

I have read the comments in your PR #19938 about additive settings and I understand the challenges about a change that would be breaking existing builds just by upgrading to the latest emscripten. So I do agree that the changes could be implemented for new settings only. Another way to deal with this, would be to have emscripten start emitting warnings for the current builds that are essentially overriding the current settings that are additive and being very clear that the behavior will change in the future...

I agree that it looks like this PR is DOA unless the additive settings is addressed as I also do not like the way it ends up looking.

[sbc100]

@sbc100 It's interesting that you are talking about the syntax, because I was thinking about this too. I feel like accepting the syntax -sPORTS=foo for a single port is weird and would make more sense if you had to write -sPORTS=[foo]. I personally am much more in favor of -sPORT=foo -sPORT=bar when you have multiple ports, since right now you have to write -sUSE_SDL=2 -sUSE_VORBIS.

I do like (even prefer) the --port or --use-port syntax as well as long as it is additive.

I have read the comments in your PR #19938 about additive settings and I understand the challenges about a change that would be breaking existing builds just by upgrading to the latest emscripten. So I do agree that the changes could be implemented for new settings only. Another way to deal with this, would be to have emscripten start emitting warnings for the current builds that are essentially overriding the current settings that are additive and being very clear that the behavior will change in the future...

I agree that it looks like this PR is DOA unless the additive settings is addressed as I also do not like the way it ends up looking.

One way to get this working today would be to add PORTS to internal_settings.py and then implement --use-port/--port via settings.PORTS.append().

I don't love the idea of add more command like options that are not settings, but it also seems like it might be path of least resistance.

[ypujante]

One way to get this working today would be to add PORTS to internal_settings.py and then implement --use-port/--port via settings.PORTS.append().

I don't love the idea of add more command like options that are not settings, but it also seems like it might be path of least resistance.

I guess I just don't think I can make this decision. I feel like, since you are not really thrilled about adding a non settings option, maybe a good comprise would be to use -sPORT (singular) and implement your APPENDING_SETTINGS concept just for this single option. It seems that a very minimal change would look like:

# settings.py
APPENDING_SETTINGS = {'PORT'}
# ...
# line 251:
    self.check_type(name, value)
    self.attrs[name] = value

# >>>> replaced with
    self.check_type(name, value)
    if name in APPENDING_SETTINGS:
      self.attrs[name].extend(value)  # self.attrs[name] and value are lists
    else:
      self.attrs[name] = value

[sbc100]

Hmm.. using -sPORT (singular) might also be confusing if its an appending setting since appending settings are lists so then we would end up allowing a list of ports even though the setting name is singular. e.g -sPORT=foo,bar.

I think adding --use-port directly may be the way to go here.

[ypujante]

Hmm.. using -sPORT (singular) might also be confusing if its an appending setting since appending settings are lists so then we would end up allowing a list of ports even though the setting name is singular. e.g -sPORT=foo,bar.

I think adding --use-port directly may be the way to go here.

I am obviously fine with --use-port, I am just aware you don't like it.

Let me suggest another alternative: what if -sPORT is a list (under the cover), but it can only take a single argument. The user has to write -sPORT=foo -sPORT=bar and the syntax -sPORT=[foo,bar] or -sPORT=foo,bar are simply invalid syntax. This way we achieve the same behavior as --use-port but with a settings?

Again I am not sure which "evil" you prefer. Just suggesting options/alternatives.

[sbc100]

Hmm.. using -sPORT (singular) might also be confusing if its an appending setting since appending settings are lists so then we would end up allowing a list of ports even though the setting name is singular. e.g -sPORT=foo,bar.
I think adding --use-port directly may be the way to go here.

I am obviously fine with --use-port, I am just aware you don't like it.

Let me suggest another alternative: what if -sPORT is a list (under the cover), but it can only take a single argument. The user has to write -sPORT=foo -sPORT=bar and the syntax -sPORT=[foo,bar] or -sPORT=foo,bar are simply invalid syntax. This way we achieve the same behavior as --use-port but with a settings?

Thats would be introducing a new kind of settings I think. Right now string settings vs list setting are simply determined by their default value in settings.js and I think this would complicate that. I can imagine having a string setting PORT and appends to a list setting (internal_settings.PORTS) but that also sounds like a bunch of new complexity.

I think, on balance, just adding --use-port is simpler.

[ypujante]

I implemented the change to use --use-port.

I took the liberty to sort the output of --show-ports since now there is a clear connection between --use-port and --show-ports as well as naming each port by the name used in --use-port to avoid confusion (for example show() in sdl2_mixer.py was returning SDL2_mixer ... and I changed it sdl2_mixer ... because that is the name of the port). Before it didn't matter because this name was irrelevant. Now that it is, I believe it is better to make it more obvious/less confusing.

Available ports:
    boost_headers - Boost headers v1.70.0 (-sUSE_BOOST_HEADERS=1 or --use-port=boost_headers; Boost license)
    bullet (-sUSE_BULLET=1 or --use-port=bullet; zlib license)
    bzip2 (-sUSE_BZIP2=1 or --use-port=bzip2; BSD license)
    cocos2d (-sUSE_COCOS2D=3 or --use-port=cocos2d)
    freetype (-sUSE_FREETYPE=1 or --use-port=freetype; freetype license)
    giflib (-sUSE_GIFLIB=1 or --use-port=giflib; MIT license)
    harfbuzz (-sUSE_HARFBUZZ=1 or --use-port=harfbuzz; MIT license)
    icu (-sUSE_ICU=1 or --use-port=icu; Unicode License)
    libjpeg (-sUSE_LIBJPEG=1 or --use-port=libjpeg; BSD license)
    libmodplug (-sUSE_MODPLUG=1 or --use-port=libmodplug; public domain)
    libpng (-sUSE_LIBPNG or --use-port=libpng; zlib license)
    mpg123 (-sUSE_MPG123=1 or --use-port=mpg123; zlib license)
    ogg (-sUSE_OGG=1 or --use-port=ogg; zlib license)
    regal (-sUSE_REGAL=1 or --use-port=regal; Regal license)
    sdl2 (-sUSE_SDL=2 or --use-port=sdl2; zlib license)
    sdl2_gfx (-sUSE_SDL_GFX=2 or --use-port=sdl2_gfx; zlib license)
    sdl2_image (-sUSE_SDL_IMAGE=2 or --use-port=sdl2_image; zlib license)
    sdl2_mixer (-sUSE_SDL_MIXER=2 or --use-port=sdl2_mixer; zlib license)
    sdl2_net (-sUSE_SDL_NET=2 or --use-port=sdl2_net; zlib license)
    sdl2_ttf (-sUSE_SDL_TTF=2 or --use-port=sdl2_ttf; zlib license)
    sqlite3 (-sUSE_SQLITE3=1 or --use-port=sqlite3); public domain)
    vorbis (-sUSE_VORBIS or --use-port=vorbis; zlib license)
    zlib (-sUSE_ZLIB=1 or --use-port=zlib; zlib license)

I did update the emcc.rst documentation and I know I have not run the command to update the documentation and as result it will not pass the test. I don't have Sphinx installed at this moment but I wanted to make sure these changes were in line with what you had in mind before I move forward with this final step...

[sbc100]

Looks good!

Perhaps we should add ChangeLog entry about this?

The instructions for installing the right version of sphinx and updating the docs are here: https://github.com/emscripten-core/emscripten/blob/main/docs/process.md#updating-the-emccpy-help-text

[ypujante]

I have made the changes you suggested (I really do like the change about keeping the ports set inside the ports module instead of in internal settings).

I am struggling with sphinx. I will try again after this push to see if I have more luck. Getting this error:

sphinx-build -b text -d build/doctrees  -W source build/text
Running Sphinx v2.4.4
/Users/ypujante/Library/Python/3.9/lib/python/site-packages/urllib3/__init__.py:35: NotOpenSSLWarning: urllib3 v2 only supports OpenSSL 1.1.1+, currently the 'ssl' module is compiled with 'LibreSSL 2.8.3'. See: https://github.com/urllib3/urllib3/issues/3020
  warnings.warn(

Sphinx version error:
The sphinxcontrib.applehelp extension used by this project needs at least Sphinx v5.0; it therefore cannot be built with this version.
make: *** [text] Error 2

[ypujante]

FTR I struggled with the dependencies in order to work with the sphinx version until I was able to make it work... had to force this in the requirements (I did NOT check that in)

sphinxcontrib-applehelp==1.0.4
sphinxcontrib-devhelp==1.0.2
sphinxcontrib.htmlhelp==2.0.1
sphinxcontrib.serializinghtml==1.1.5
sphinxcontrib.qthelp==1.0.3
alabaster==0.7.1

[ypujante]

Yay. All passing!

[sbc100]

Can you also update site/source/docs/compiling/Building-Projects.rst? If this is going to be the primary mechanism maybe it should be used everywhere in that file (with a mention of the legacy settings in the note perhaps).

[sbc100]

Can you give a little more detail.

How about:

Added `--use-port` option to `emcc`.  This option allows ports to be enabled by name
and is designed to replace all the exist `-sUSE_XXX` settings for ports. ....

Also, can you end the entry with (#21214)?

[ypujante]

Sure I will. I think it makes sense to emphasize that it will replace the -sUSE_XXX syntax eventually. Also I would imagine that no new "official" port (unclear if there is any in the pipe), should add -sUSE_XXX and instead use --use-port syntax... Certainly contrib ports will.

After seeing the end result, I am quite happy with the new syntax which is in-line with the other --clear-ports, --show-ports and is essentially a way to add new ports, whether official or contrib in a more generic fashion without adding new settings...

After this last change I will be working on contrib port proper

[sbc100]

I've been holding off adding any new official ports for several years now, until we had this contrib system in place.

[ypujante]

@sbc100 I made the documentation changes you requested. A couple of things:

• I took the liberty to fix the code example that was pointing to an invalid file and made sure that the command provided as an example actually work
• Based on the code, I "fixed" the documentation that was mentioning the sdl2 image formats since it looks like now jpg is supported: here is the code (sdl2_image.py)

def process_dependencies(settings):
  settings.USE_SDL = 2
  if 'png' in settings.SDL2_IMAGE_FORMATS:
    deps.append('libpng')
    settings.USE_LIBPNG = 1
  if 'jpg' in settings.SDL2_IMAGE_FORMATS:
    deps.append('libjpeg')
    settings.USE_LIBJPEG = 1

Finally, I know you were not to too excited about port options, but clearly sdl2_image is a good example showing that port options are actually useful/necessary. With the new syntax in place, I have a few ideas, like for example allowing a syntax like : --use-port=sdl2_image?bmp&jpg or --use-port=sdl2_image?formats=bmp,jpg (query string style) or using a ':' instead like --use-port=sdl2_image:bmp&jpg or --use-port=sdl2_image:formats=bmp,jpg, and other variants of these... Also you could imagine that each port could just handle its own options so emscripten is not involved...

In any case, the next phase (part 2) will be about adding contrib ports and I will not be adding port options until part 3 anyway...