Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate STRICT from MINIMAL_RUNTIME #8411

Merged
merged 1 commit into from
Apr 9, 2019
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Apr 8, 2019

This allows us to add more incompatibility checking to the STRICT
option without forcing it on all users or MINIMAL_RUNTIME.

This is part of #8317.

In particular we already already error on the use legacy settings
when in STRICT mode, but I understand that at least on major user (@juj)
wants to be able to continue to use legacy command line settings and
also wants to use MINIMAL_RUNTIME.

@sbc100 sbc100 requested review from kripken and juj April 8, 2019 17:09
@sbc100 sbc100 force-pushed the split_strict_minimal branch 2 times, most recently from 71ce045 to c2f8909 Compare April 8, 2019 17:24
This allows us to add more incompatibility checking to the STRICT
option without forcing it on all users or MINIMAL_RUNTIME.

This is part of #8317.

In particular we already already error on the use legacy settings
when in STRICT mode, but I understand that at least on major user (@juj)
wants to be able to continue to use legacy command line settings and
also wants to use MINIMAL_RUNTIME.
@sbc100 sbc100 merged commit cb33ba9 into incoming Apr 9, 2019
@sbc100 sbc100 deleted the split_strict_minimal branch April 9, 2019 18:24
@juj
Copy link
Collaborator

juj commented Apr 12, 2019

Initially STRICT option was added as a way to users to opt in early to breaking changes, before they are made default. The first batch of breaking changes was to
a) migrate to ERROR_ON_UNDEFINED_SYMBOLS from 0->1 by default,
b) ERROR_ON_MISSING_LIBRARIES from 0->1 by default, and to
c) enforce that we do not by default link to all JS libraries, but instead one would have to pass -llibrary_system_lib.js (or in case where an alias exists, then something like -lGL).

The two first features have a dedicated option for them, but the last feature never got any other option flag, but it was made part of STRICT from the get go, i.e. there is no other way to adjust the system lib linking inclusing behavior than to enable STRICT. (in hindsight it probably should have had a way)

The idea was that these options would briefly live under STRICT mode, and then be migrated to become the default behavior. This occurred for e.g. ERROR_ON_UNDEFINED_SYMBOLS=1, but it did not happen for ERROR_ON_MISSING_LIBRARIES which still defaults to 0. And the transition did not occur for strict system lib linking behavior either.

If at some point of time no migrations would be ongoing, then enabling STRICT would not be doing anything.

At least, that was the original idea. The reason was that people would have an easy way to opt in (e.g. for testing purposes) to whatever the latest set of changes would be. But then later, we did not get any more new features that we would have liked to label under STRICT, so it then kind of stuck to mean just those things above, a)-c).

In hindsight, having a compiler option that would mean different things at different times was not a great one (except for local testing, i.e. one could briefly add STRICT to a local run of their own project to double check if their project is compatible with the latest upcoming compiler version). So to avoid it mutating meaning, we just kept it to mean the fixed set of things listed above, and I paired it to MINIMAL_RUNTIME by default.

Now that STRICT also means "you cannot specify the option that only has one supported (but deprecated) value" after #8261, it means that we cannot simultaneously enable strict system lib linking, while retaining compatibility with multiple (compatible/identically behaving) Emscripten versions, since #8261 enforces that as build system developers we would have to spend time to do build system plumbing that gives no benefits. (Having to do extra work to support backward compatibility for features that change is ok, but having to do extra work to support backwards compatibility when there is no circumstance that would cause a backwards incompatibility is a waste of developer time)

The reason why STRICT was initially paired with MINIMAL_RUNTIME was to enforce that users of MINIMAL_RUNTIME would not get the old legacy lazy "link to all src/library_*.js" behavior, since adopting MINIMAL_RUNTIME many of those no longer applied, and the spirit with MINIMAL_RUNTIME is that developers would give a new look to which code they are pulling in.

When we use MINIMAL_RUNTIME, we would like to have the strict system lib linking behavior, but not the "cannot specify options that only have one supported (but deprecated) value" that we see to be unuseful and counterproductive. In that sight, we would now like to not enable STRICT since it has that added meaning, but have some other option to opt in to strict system lib linking behavior when MINIMAL_RUNTIME is used.

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 15, 2019

As far as I can tell this change achieves exactly what you are suggesting in the last paragraph. It means that anyone using MINIMAL_RUNTIME will get the "don't auto-link JS libraries" without getting the "cannot specify legacy options".

Another way of putting it: change removes the "automatically-use-STRICT" from MINIMAL_RUNTIME while maintains the "don't auto-link js libraries" behaviour that you want from MINIMAL_RUNTIME.

Do is there another change that needs to be made? Perhaps we could be more explicit and create MINIMAL_JS_LIBS or NO_AUTO_JS_LIBS option if you would prefer and have both MINIMAL_RUNTIME and STRICT imply that?

VirtualTim pushed a commit to VirtualTim/emscripten that referenced this pull request May 21, 2019
This allows us to add more incompatibility checking to the STRICT
option without forcing it on all users or MINIMAL_RUNTIME.

This is part of emscripten-core#8317.

In particular we already already error on the use legacy settings
when in STRICT mode, but I understand that at least on major user (@juj)
wants to be able to continue to use legacy command line settings and
also wants to use MINIMAL_RUNTIME.
VirtualTim added a commit to VirtualTim/emscripten that referenced this pull request May 23, 2019
VirtualTim added a commit to VirtualTim/emscripten that referenced this pull request May 23, 2019
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
This allows us to add more incompatibility checking to the STRICT
option without forcing it on all users or MINIMAL_RUNTIME.

This is part of emscripten-core#8317.

In particular we already already error on the use legacy settings
when in STRICT mode, but I understand that at least on major user (@juj)
wants to be able to continue to use legacy command line settings and
also wants to use MINIMAL_RUNTIME.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants