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

Disallow EMTEST_SAVE_DIR, which has not worked in mode 2 #22178

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

kripken
Copy link
Member

@kripken kripken commented Jul 2, 2024

At some point mode 2 there broke. To avoid confusion, remove the env var entirely,
and provide a clear error message pointing people to the proper options.

@kripken kripken requested review from sbc100 and aheejin July 2, 2024 22:15
@kripken
Copy link
Member Author

kripken commented Jul 2, 2024

(I think mode 2 broke because configure() is called twice, and we read and wrote the env var for internal reasons, so we trampled the state. To avoid that, this PR makes us not use the env var as internal state at all.)

@aheejin
Copy link
Member

aheejin commented Jul 2, 2024

Do we have any comment on what these values (0/1/2) for EMTEST_SAVE_DIR mean? I just empirically used to use --save-dir --no-clean because it worked, but I'm not sure what these values mean. Perhaps we can add some comments on its definition? (If this is a user-facing option, adding an entry in the doc would be better, but I guess it's mostly used by us?)

@kripken
Copy link
Member Author

kripken commented Jul 2, 2024

Those values are not user-facing after this PR, and maybe I should refactor the internals to avoid them..? I wasn't sure how to rename them though in a way that is consistent with the ones around them.

But, they just mean 1 = save-dir, 2 = no-clean (which also implies save-dir, so note that you don't need both).

@kripken kripken merged commit 2df5d9e into emscripten-core:main Jul 3, 2024
28 checks passed
@kripken kripken deleted the emtest_save_dir branch July 3, 2024 17:23
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