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

Fix: Accept only option flags in CRYSTAL_OPTS for build commands #11922

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

beta-ziliani
Copy link
Member

@beta-ziliani beta-ziliani commented Mar 23, 2022

This is the reverted pr #11780 together with suggested fix #11921

Supersedes #11921 and #11780
Fixes #11164

@beta-ziliani beta-ziliani requested a review from HertzDevil March 23, 2022 12:39
@straight-shoota
Copy link
Member

The use of CRYSTAL_OPTS with spec and eval is a bit troublesome. They only support a subset of compiler options and fail with CRYSTAL_OPTS=--warn-on-errors (because --warn-on-errors is no part of setup_simple_compiler_options (it probably should, though).
Anyways, the change is fine. This was broken before and it's still broken, so nothing changes.
But I think we should refactor the entire compiler command options parsing process. It's very obscure which compiler command actually accepts which options. And eventually, we should have a fixed set of build options that's allowed for CRYSTAL_OPTS and should work on any command. If the command doesn't use a build option, but it's allowed for CRYSTAL_OPTS, it should just be ignored.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler labels Mar 25, 2022
@beta-ziliani beta-ziliani added this to the 1.4.0 milestone Mar 25, 2022
@HertzDevil
Copy link
Contributor

Ignoring unknown option flags at the OptionParser level could affect subsequent parsing, because they never consume an extra argument token, so those unsupported flags should be explicit no-ops; they could in turn be supported even for usual command-line arguments if we document the no-ops as such.

To me all the extra behavior suggests it could use a better name than just CRYSTAL_OPTS. I prefer the differences between command-line arguments and CRYSTAL_OPTS to be as minimal as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CRYSTAL_OPTS messes with positional CLI arguments
3 participants