Add options support to contrib ports (contrib ports part 3)

[ypujante]

This is hopefully the last part of contrib ports: adding the ability to support options.

I have deliberately kept the design very simple and minimal on the emscripten side (just 11 line code change).
The basic idea is to allow options after the port name: --use-port=<port>:<options>.

Here are a few examples that show it in action:

./emcc "--use-port=contrib.glfw3:disableWarning=true:disableJoystick=true" ./test/hello_world.c -o /tmp/out.js
./emcc "--use-port=contrib.glfw3:tag=1.0.3" ./test/hello_world.c -o /tmp/out.js
./emcc "--use-port=sdl2_image:formats=jpg,png" ./test/hello_world.c -o /tmp/out.js

The last example is, IMO, a lot more concise than -sUSE_SDL_IMAGE=2 -sSDL2_IMAGE_FORMATS=["jpg", "png"] and also less error prone since the options for the port are with the port definition itself not in a separate setting.

From an implementation point of view on the emscripten side, it is very minimal/simple: look for ":" and everything after is considered to be an option and is 100% delegated to the port to handle via the function handle_options(options). In other words, "options" is treated as a black box from emscripten point of view and it is the responsibility of the port to handle them the way it wants. Which leads to a very simple implementation:

def handle_use_port_arg(settings, arg):
  args = arg.split(':', 1)
  name, options = args[0], args[1] if len(args) == 2 else None
  if name not in ports_by_name:
    utils.exit_with_error(f'Invalid port name: {name} used with --use-port')
  ports_needed.add(name)
  if options:
    port = ports_by_name[name]
    if not hasattr(port, 'handle_options'):
      utils.exit_with_error(f'Invalid options for port {name}: No options available')
    else:
      error = port.handle_options(options)
      if error is not None:
        utils.exit_with_error(f'Invalid options for port {name}: {error}')

For error handling I chose to have handle_options returns None if everyting Ok or an error string in case of error, but I am open to other ways of handling errors. The port could call exit_with_error instead but I thought that was a bit harsh...

I chose "?" as a separator because it is often used for optional things and also because it invokes a query string which options usually look like. But I am happy to use a different separator (like ":" or whatever).

Although very minimal, I believe this is quite powerful and readable/understandable from a user point of view (of course it is the responsibility of the port developer to document the options and what they mean/do). As you can see from what I did in contrib.glfw3, simply making tag an option allow the use a different version of the port itself without requiring an emscripten PR which I think is quite nice!

Note that the changes I made in sdl2_image.py and test_browser.py are for illustration purposes only and to show that even the 2 syntax can coexist at the moment (especially since now it is possible to use --use-port=sdl2_image). But I was planning to revert this changes back unless of course you think it is worth keeping. Or I can make changes to other ports that have options as well. It is up to you.

I have also not updated any documentation until there is an agreement on the changes at which I will document it thoroughly.

[sbc100]

Somewhat unrelated to change particular change, but for the builtin ports we have the variants map which allows us to enumerate all possible variants. Maybe it would be to continue to support that?

Also, I'm not sure we want to support installing more than tag / release at a time. In general, it keeps things much simpler if we support just one version of a given library at a given time. For now, I think its best if folks who want to play with different versions have to edit the port file.

[ypujante]

Somewhat unrelated to change particular change, but for the builtin ports we have the variants map which allows us to enumerate all possible variants. Maybe it would be to continue to support that?

I do not know how the concept of variants work exactly. From my code search I understood that the only place it is actually used is in embuilder. I don't think there is a way to use the sdl2-mt variant with emcc unless I am wrong. We could certainly extend the --use-port syntax to allow for variant names as well (--use-port=sdl2-mt), but that would probably be another PR. I am actually not sure what the code would look like because embuilder uses some complex code with @contextmanager for variants and I am very unfamiliar with this.

Also, I'm not sure we want to support installing more than tag / release at a time. In general, it keeps things much simpler if we support just one version of a given library at a given time. For now, I think its best if folks who want to play with different versions have to edit the port file.

This is a real bummer because I think this is super powerful, but that is fine.

[sbc100]

Somewhat unrelated to change particular change, but for the builtin ports we have the variants map which allows us to enumerate all possible variants. Maybe it would be to continue to support that?

I do not know how the concept of variants work exactly. From my code search I understood that the only place it is actually used is in embuilder. I don't think there is a way to use the sdl2-mt variant with emcc unless I am wrong. We could certainly extend the --use-port syntax to allow for variant names as well (--use-port=sdl2-mt), but that would probably be another PR. I am actually not sure what the code would look like because embuilder uses some complex code with @contextmanager for variants and I am very unfamiliar with this.

Also, I'm not sure we want to support installing more than tag / release at a time. In general, it keeps things much simpler if we support just one version of a given library at a given time. For now, I think its best if folks who want to play with different versions have to edit the port file.

This is a real bummer because I think this is super powerful, but that is fine.

Once we have the contrib ports system up and running perhaps we can reconsider then. Lets see who (aside from yourself) starts finding it useful and how.

[ypujante]

I have made the changes you suggested, which has the neat advantage of making the code in the ports simpler. And of course the code in __init.py__ is slightly more complicated.

I added the constraint on OPTIONS if the port supports options so that we can display a useful error message in case of errors:

./emcc "--use-port=sdl2_image?xx=3" ./test/hello_world.c -o /tmp/out.js
emcc: error: xx=3 is not valid. Available options are {'formats': 'A comma separated list of formats (ex: png,jpg)'}.

Do you want me to keep the changes in sdl2_image? I made them for illustration purposes but if you think they are useful I can keep them (and also add them to other ports like sdl2_mixer).

[ypujante]

I cannot figure out why I am getting this error on CI:

test_create_readonly (test_other.other) ... ok (2.03s)
emcc: error: undefined exported symbol: "\\ '_c'" [-Wundefined] [-Werror]

EXPORTED_FUNCTIONS=["_a", "_b", \"_c", "_d"]
Traceback (most recent call last):
  File "/root/project/em++.py", line 15, in <module>
    sys.exit(emcc.main(sys.argv))
  File "/usr/lib/python3.6/contextlib.py", line 52, in inner
    return func(*args, **kwds)
  File "/root/project/emcc.py", line 1567, in main
    ret = run(args)
  File "/root/project/emcc.py", line 632, in run
    linker_inputs = phase_compile_inputs(options, state, newargs, input_files)
  File "/usr/lib/python3.6/contextlib.py", line 52, in inner
    return func(*args, **kwds)
  File "/root/project/emcc.py", line 1045, in phase_compile_inputs
    compile_source_file(i, input_file)
  File "/root/project/emcc.py", line 1022, in compile_source_file
    cmd = get_clang_command()
  File "/root/project/emcc.py", line 949, in get_clang_command
    return compiler + get_cflags(state.orig_args, use_cxx) + compile_args
  File "/root/project/emcc.py", line 425, in get_cflags
    ports.add_cflags(cflags, settings)
  File "/root/project/tools/ports/__init__.py", line 476, in add_cflags
    port.get(Ports, settings, shared)
  File "/root/project/tools/ports/contrib/glfw3.py", line 66, in get
    return [shared.cache.get_lib(get_lib_name(settings), create, what='port')]
  File "/root/project/tools/cache.py", line 139, in get_lib
    return get(name, *args, **kwargs)
  File "/root/project/tools/cache.py", line 155, in get
    raise Exception(f'FROZEN_CACHE is set, but cache file is missing: "{shortname}" (in cache root path "{cachedir}")')
Exception: FROZEN_CACHE is set, but cache file is missing: "sysroot/lib/wasm32-emscripten/lib_contrib.glfw3-nw.a" (in cache root path "/root/cache")

I am not getting this error locally:

./test/runner other.test_contrib_ports
Test suites:
['test_other']
Running test_other: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
test_contrib_ports (test_other.other) ... ok

----------------------------------------------------------------------
Ran 1 test in 0.441s

[sbc100]

This error is because we run the tests with frozen cache enabled. We build all the libraries needed for the tests using ./embuilder build MINIMAL and then we share the result sysroot across all the test runners.

See .circleci/config.yml and the of frozen cache.

Normally when we add new libraries like this we add them to the MINIMAL set in embuilder. In this case we probably don't want to do that though. Maybe instead we should move this test out into test_sanity.py (which doesn't run with frozen cache).

[sbc100]

This error is because we run the tests with frozen cache enabled. We build all the libraries needed for the tests using ./embuilder build MINIMAL and then we share the result sysroot across all the test runners.

See .circleci/config.yml and the of frozen cache.

Normally when we add new libraries like this we add them to the MINIMAL set in embuilder. In this case we probably don't want to do that though. Maybe instead we should move this test out into test_sanity.py (which doesn't run with frozen cache).

There are also a bunch of other tests that have these two lines:

if config.FROZEN_CACHE:
      self.skipTest("test doesn't work with frozen cache")

You could add those lines.. but then also mark the test as @crossplatform where it will at least run on at least the windows runner.

[ypujante]

I would never have found out what was going on with the frozen cache!

So here is what I did to fix the issue:

1. I reverted the test in other to not use options at all since it is working without options
2. since browser.test_sdl2_image_formats has a test that uses options, then we do have a test that uses options

[ypujante]

@sbc100 In regards to the syntax, it's true that if it not quoted the shell output can get quite ugly and hard to understand what is going on.

Here is a couple of alternatives that are shell friendly:

1. Use : only => ex: --use-port=contrib.glfw3:disableWarning=true:disableJoystick=true (this can work because the first : separate the port name from the "query string", the remainder gets parsed separately)
2. Use @ and : => ex: --use-port=contrib.glfw3@disableWarning=true:disableJoystick=true (@ separate the port name from the options, : separate each option)

Both of these syntax would work. I only have a very slight preference for 2 just for a clearer separation between the port name and the options, but I would be fine with either one.

From an implementation point of view, either of these syntax are identical and also super trivial to implement since parse_qs accepts a separator character that can be set to :

What do you think?

[sbc100]

@sbc100 In regards to the syntax, it's true that if it not quoted the shell output can get quite ugly and hard to understand what is going on.

Here is a couple of alternatives that are shell friendly:

1. Use : only => ex: --use-port=contrib.glfw3:disableWarning=true:disableJoystick=true (this can work because the first : separate the port name from the "query string", the remainder gets parsed separately)
2. Use @ and : => ex: --use-port=contrib.glfw3@disableWarning=true:disableJoystick=true (@ separate the port name from the options, : separate each option)

Both of these syntax would work. I only have a very slight preference for 2 just for a clearer separation between the port name and the options, but I would be fine with either one.

From an implementation point of view, either of these syntax are identical and also super trivial to implement since parse_qs accepts a separator character that can be set to :

What do you think?

Either one seems pretty reasonable to me.

[sbc100]

lgtm!

Can you update the PR description to match the new syntax?

[ypujante]

@sbc100 Sounds great.

I am also going to update the documentation (the README.md for contrib ports at least). I am not sure it is necessary to put it in the Changelog unless you think I should (or maybe just update the prior Changelog entry?)

Also do you want me to make a similar change to sdl2_mixer (since this one has options too) or maybe another PR?

[sbc100]

@sbc100 Sounds great.

I am also going to update the documentation (the README.md for contrib ports at least). I am not sure it is necessary to put it in the Changelog unless you think I should (or maybe just update the prior Changelog entry?)

Also do you want me to make a similar change to sdl2_mixer (since this one has options too) or maybe another PR?

Updating the existing changelog entry sounds good. No need to add sdl2_mixer to this change unless you want to.

[ypujante]

I have updated the documentation (provided details in README.md). Since I was updating Building-Projects.rst to use the new syntax (--use-port=sdl2_image:formats=bmp,png,xpm,jpg) I took the liberty to reformat the notes around to be 80 columns max since this is what you asked me to do in a prior PR. I can revert this if not ok.

[ypujante]

@sbc100 Finally I wanted to elaborate on your comment regarding tag:

Also, I'm not sure we want to support installing more than tag / release at a time

I believe there are 2 use cases:

1. The current version of emscripten uses sdl2 2.24.2 yet the current version of sdl2 available is 2.30.0. If I want to use sdl2 2.30.0 because of a new feature or a bug in 2.24.2, there is no easy way for me to do that (submitting a PR to emscripten is pretty involved and not a lot of people would do it)
2. Now let's say I use the current version of emscripten which has sdl2 2.24.2 and my code works great. Now let's say that emscripten gets updated with sdl2 2.30.0. I upgrade emscripten and suddenly my code doesn't work anymore because let's say 2.30.0 has a bug that was not present in 2.42.2. I cannot easily revert back. Or another way to put it I cannot "lock" the version of sdl2 that was used. It is now tied to emscripten.

These 2 use cases could easily be solved with tag being an option. Obviously IF the "instructions" to build the new version are the same but for "point" releases it should be...

It's fine to not do it part of this PR but I wanted to keep it in the back of you head because I don't think these are far fetched use cases. Locking a version is a fairly common thing to do... after all ,emscripten documentation requires an antiquated version of sphinx to generate it (which side note, is kind of a PITA to install...)

[sbc100]

Regarding multi-versioning for ports, I think it sounds complicated and error prone and outside the scope of what the builtin ports can/should be. For something more fully featured I think we should recommend something like https://vcpkg.io/en/ which is more fully formed and designed for such things. We have talking about better integresion with vcpkg in the past (or conan perhaps). That way the packaging system can be separate from the compiler itself.

Regarding the version of sphinx, I would love to upgrade but I've tried and to do it a couple of times. I think making it work with the latest sphinx would require some digging into the internal details of sphinx. Even if we did upgrade though , we would certainly still want to pin the version we use so the that output we get is the same regardless of who builds the docs.

[ypujante]

@sbc100 There is an error in the tests TypeError: parse_qs() got an unexpected keyword argument 'separator'

From my understanding this was introduced in python 3.16.13:

Changed in version 3.6.13: Added separator parameter with the default value of &. Python versions earlier than Python 3.6.13 allowed using both ; and & as query parameter separator. This has been changed to allow only a single separator key, with & as the default separator.

I am not sure what the requirements for python3. I can change the code to do this instead:

# options_qs = parse_qs(options, strict_parsing=True, separator=':')
options_qs = parse_qs(options.replace(':', '&'), strict_parsing=True)

It does have the drawback (or shall I say advantage) that using this --use-port=contrib.glfw3:disableWarning=true&disableJoystick=true works when it would fail with the separator option...

I need your guidance there...

[sbc100]

@sbc100 There is an error in the tests TypeError: parse_qs() got an unexpected keyword argument 'separator'

From my understanding this was introduced in python 3.16.13:

Changed in version 3.6.13: Added separator parameter with the default value of &. Python versions earlier than Python 3.6.13 allowed using both ; and & as query parameter separator. This has been changed to allow only a single separator key, with & as the default separator.

I am not sure what the requirements for python3. I can change the code to do this instead:

# options_qs = parse_qs(options, strict_parsing=True, separator=':')
options_qs = parse_qs(options.replace(':', '&'), strict_parsing=True)

It does have the drawback (or shall I say advantage) that using this --use-port=contrib.glfw3:disableWarning=true&disableJoystick=true works when it would fail with the separator option...

I need your guidance there...

How about we just avoid parse_qs completely (its specifically designed for URLs, and this isn't one) and just parse the string ourselves with str.split, etc?

[ypujante]

How about we just avoid parse_qs completely (its specifically designed for URLs, and this isn't one) and just parse the string ourselves with str.split, etc?

The issue is that parse_qs does more than just splitting and we would have to re-implement it: handle escaping (%xx syntax) and option repeat (from the README I updated):

--use-port=contrib.foo:bar=1:bar=2%263 => calls tools.ports.contrib.foo.handle_options({'bar': ['1', '2&3']})

[sbc100]

How about we just avoid parse_qs completely (its specifically designed for URLs, and this isn't one) and just parse the string ourselves with str.split, etc?

The issue is that parse_qs does more than just splitting and we would have to re-implement it: handle escaping (%xx syntax) and option repeat (from the README I updated):

--use-port=contrib.foo:bar=1:bar=2%263 => calls tools.ports.contrib.foo.handle_options({'bar': ['1', '2&3']})

How about we only handle simple strings.. no escaping or fancy stuff?

[ypujante]

How about we only handle simple strings.. no escaping or fancy stuff?

I wouldn't call the ability to write format=jpg:format=png fancy personally...

I know how to fix it without separator and drawback I mentioned...

[sbc100]

How about we only handle simple strings.. no escaping or fancy stuff?

I wouldn't call the ability to write format=jpg:format=png fancy personally...

How about we only support format=jpg,png. And that repeating settings don't combine. That does not seem unreasonable to me

[ypujante]

How about we only support format=jpg,png. And that repeating settings don't combine. That does not seem unreasonable to me

Sure but we just went through a whole argument about how it was annoying that we could not use -sUSE_PORT multiple times and that we did not have this issue with --use-port so it seems limiting to not support it there when it is simple to do it and we just don't know how ports will be used in the future...

the code looks like this:

# options_qs = parse_qs(options, strict_parsing=True, separator=':')
options_qs = options.replace('&', '%26').replace(':', '&') 
options_qs = parse_qs(options_qs, strict_parsing=True)

But if you are really really opposed to these small change, then I will do whatever you want to move forward

[sbc100]

How about we only support format=jpg,png. And that repeating settings don't combine. That does not seem unreasonable to me

Sure but we just went through a whole argument about how it was annoying that we could not use -sUSE_PORT multiple times and that we did not have this issue with --use-port so it seems limiting to not support it there when it is simple to do it and we just don't know how ports will be used in the future...

It makes to me that you should be able to specify a sequence of different ports by adding --use-port multiple times.

It makes way less sense to me to specify that same port more than once on the same command line with different options. E.g. What would it mean to do --use-port=foo:enableX following by --use-port=foo:disableX?

the code looks like this:

# options_qs = parse_qs(options, strict_parsing=True, separator=':')
options_qs = options.replace('&', '%26').replace(':', '&') 
options_qs = parse_qs(options_qs, strict_parsing=True)

But if you are really really opposed to these small change, then I will do whatever you want to move forward

Seems reasonable to me.

I do worry a little that we will end up supporting all kind of weird escape characters from URL syntax on our command line, and then people might start relying on it, making it hard to remove/refactor in the future. In general, I prefer to limit what we accept so that we limit what we end up needing to support and test.

[sbc100]

Regarding multi-versioning for ports, I think it sounds complicated and error prone and outside the scope of what the builtin ports can/should be. For something more fully featured I think we should recommend something like https://vcpkg.io/en/ which is more fully formed and designed for such things. We have talking about better integresion with vcpkg in the past (or conan perhaps). That way the packaging system can be separate from the compiler itself.

Regarding the version of sphinx, I would love to upgrade but I've tried and to do it a couple of times. I think making it work with the latest sphinx would require some digging into the internal details of sphinx. Even if we did upgrade though , we would certainly still want to pin the version we use so the that output we get is the same regardless of who builds the docs.

Oh, we also have mechanism (which is not really documented or tested) called EMCC_LOCAL_PORTS .. its allows you use custom versions of ports:

emscripten/tools/ports/__init__.py

Lines 241 to 253 in 2a00e26

	# EMCC_LOCAL_PORTS: A hacky way to use a local directory for a port. This
	# is not tested but can be useful for debugging
	# changes to a port.
	#
	# if EMCC_LOCAL_PORTS is set, we use a local directory as our ports. This is useful
	# for testing. This env var should be in format
	# name=dir,name=dir
	# e.g.
	# sdl2=/home/username/dev/ports/SDL2
	# so you could run
	# EMCC_LOCAL_PORTS="sdl2=/home/alon/Dev/ports/SDL2" ./test/runner.py browser.test_sdl2_mouse
	# this will simply copy that directory into the ports directory for sdl2, and use that. It also
	# clears the build, so that it is rebuilt from that source.

Maybe if we improved/documented that it would allow folks like to achieve custom versions?

[ypujante]

Maybe if we improved/documented that it would allow folks like to achieve custom versions?

I did not know about this but that is very interesting indeed! I agree it would be great to improve/document it and make it officially "not hacky" :)

This mechanism totally eliminates the need to support tag as an option! Thanks for pointing it out.

[ypujante]

@sbc100 Due to your concerns, I have simplified the syntax for options to not use parse_qs anymore so there is no more character escaping or duplicate options supported. This obviously makes the code in __init.py__ marginally more complicated since I have to implement some of what parse_qs was doing.

I have also taken this opportunity to add a test in other.test_use_port_errors since there are many error cases possible and making sure there is a test that cover them all.

[ypujante]

Thank you @sbc100

I am going to spend a little of time investigating EMCC_LOCAL_PORTS because I find this very intriguing.