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

Set --run args only when we're actually running #11524

Merged
merged 3 commits into from
Jan 30, 2024
Merged

Conversation

Simn
Copy link
Member

@Simn Simn commented Jan 30, 2024

This changes the behavior of Sys.args() to what @tobil4sk suggests in #11087 (comment).

Doesn't add a test yet because our sys tests require a PHD.

@Simn
Copy link
Member Author

Simn commented Jan 30, 2024

Also, the fact that this doesn't cause any sys test failures is probably concerning...

Edit: Actually it looks like these tests exclusively check --run, so it's expected to not break anything there. In that case all we really need to test is that macros don't have the runtime args on any target.

@tobil4sk
Copy link
Member

In that case all we really need to test is that macros don't have the runtime args on any target.

Note, the only possible target where runtime args can be available at macro time is eval, because it is always compiled and run together. For other targets, compilation and execution are completely separate, so there is no way the macro could know what cli arguments the generated code will be run with.

Maybe this would make more sense as an eval-specific test.

@Simn
Copy link
Member Author

Simn commented Jan 30, 2024

I was thinking of some crazy case where we would leak com.args from --run to the macro context, but yeah, that doesn't seem like it would be possible.

@tobil4sk
Copy link
Member

Maybe worth adding a test for --interp as well, so we know the compiler flags don't bleed through into runtime.

@Simn Simn merged commit b0ec862 into development Jan 30, 2024
121 of 122 checks passed
@Simn Simn deleted the sys_args_cleanup branch January 30, 2024 20:43
@skial skial mentioned this pull request Feb 1, 2024
1 task
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.

2 participants