Skip to content

Allow setting of individual config settings via -s flags#12608

Closed
sbc100 wants to merge 1 commit intomasterfrom
command_line_config
Closed

Allow setting of individual config settings via -s flags#12608
sbc100 wants to merge 1 commit intomasterfrom
command_line_config

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 26, 2020

This means that the config setting COMPILER_WRAPPER can
also be set on the command line via -s COMPILER_WRAPPER='foo'.

Caveat: Using this feature ends up overriding he config setting
fairly late in the process which is too late for certain settings
to be overridden (for example -sLLVM_ROOT won't work because its
value is mostly used during startup when shared.py is first
imported. I'm have some factors planning to make this less
of a problem).

Fixes: #12340

@sbc100 sbc100 requested a review from kripken October 26, 2020 23:33
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 26, 2020

@pfaffe

Base automatically changed from settings_flag_refactor to master October 27, 2020 00:09
This means that the config setting `COMPILER_WRAPPER` can
also be set on the command line via `-s COMPILER_WRAPPER='foo'`.

Caveat: Using this feature ends up overriding he config setting
fairly late in the process which is too late for certain settings
to be overridden (for example -sLLVM_ROOT won't work because its
value is module used during startup when shared.py is first
imported.  I'm have some factors planning to make this less
of a problem).

Fixes: #12340
@sbc100 sbc100 force-pushed the command_line_config branch from e4ddca8 to 67526d9 Compare October 27, 2020 00:15
@pfaffe
Copy link
Collaborator

pfaffe commented Oct 27, 2020

After this change, what is the precedence order of settings? Config < Environment Variable < Commandline?

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 27, 2020

Yup.

@pfaffe
Copy link
Collaborator

pfaffe commented Oct 27, 2020

I'm not sure if this is in the scope of this PR, but would it be possible to include a switch to ignore environment variables altogether?
Settings passed in via the environment become 'opt out', in the sense that if you want to be sure in a build script that a specific option is not set to something unexpected you have to override it on the command line. That makes it hard to create a hermetic build setup, since you never know what the developer's environment contains that might affect your build.

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 27, 2020

That sounds unrelated to this PR yes. Maybe open an issue for that? We can discuss further there.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caveat here worries me.

Perhaps we could do a refactoring before this PR to avoid capturing these variables anywhere, so that when we use them, at least, we always look at the latest value?


if 'EMMAKEN_COMPILER' in os.environ:
diagnostics.warning('deprecated', '`EMMAKEN_COMPILER` is deprecated.\n'
'To use an alteranative LLVM build set `LLVM_ROOT` in the config file (or `EM_LLVM_ROOT` env var).\n'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'To use an alteranative LLVM build set `LLVM_ROOT` in the config file (or `EM_LLVM_ROOT` env var).\n'
'To use an alternative LLVM build, set `LLVM_ROOT` in the config file (or `EM_LLVM_ROOT` env var).\n'

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 29, 2020

I do have plans, and have tried a few times in the past to change the way startup works to avoid capturing these variables on python load... but I don't have PR for that yet.

How about if we limit this to just COMPILER_WRAPPER for now.. we can extend it to other settings if there is demand and after a refactor.

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 30, 2020

Closing in favor of a less broad solution: #12662

@sbc100 sbc100 closed this Oct 30, 2020
@sbc100 sbc100 deleted the command_line_config branch October 31, 2020 15:19
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.

Replacement for deprecated EMMAKEN_COMPILER?

3 participants