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

Accept old mixed-case toolchain_env_var variable names #729

Merged
merged 1 commit into from Nov 16, 2022

Conversation

adamgreig
Copy link
Contributor

See #728 and #687.

Having done this, though, I wonder if having the toolchain env vars be uppercase would be better in general, even if we maintain an old/deprecated mixed-case name? All the tool overrides are already in uppercase, and using all-uppercase environment variables is a widely used convention. We could extend deprecated_toolchain_env_var to cover both NMIGEN_ENV_Diamond and AMARANTH_ENV_Diamond, with AMARANTH_ENV_DIAMOND being the nominal name, used first if multiple are set.

I don't especially mind, happy to just take this as-is (or with whatever changes you want), but maybe it would be more consistent to deliberately change to all-uppercase for 0.4.

@whitequark
Copy link
Member

We could extend deprecated_toolchain_env_var to cover both NMIGEN_ENV_Diamond and AMARANTH_ENV_Diamond, with AMARANTH_ENV_DIAMOND being the nominal name, used first if multiple are set.

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2022

Codecov Report

Merging #729 (a7dfa3e) into main (db24a14) will decrease coverage by 0.01%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##             main     #729      +/-   ##
==========================================
- Coverage   81.17%   81.15%   -0.02%     
==========================================
  Files          49       49              
  Lines        6535     6539       +4     
  Branches     1567     1569       +2     
==========================================
+ Hits         5305     5307       +2     
- Misses       1037     1038       +1     
- Partials      193      194       +1     
Impacted Files Coverage Δ
amaranth/build/plat.py 26.31% <37.50%> (+0.33%) ⬆️
amaranth/build/run.py 21.73% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Accepts previous case for backwards compatibility.

Fixes amaranth-lang#728.
@adamgreig adamgreig changed the title Don't uppercase toolchain in toolchain_env_var. Fixes #728. Use all-uppercase toolchain_env_var names, but accept previous case for back-compat Nov 13, 2022
@adamgreig adamgreig changed the title Use all-uppercase toolchain_env_var names, but accept previous case for back-compat Accept old mixed-case toolchain_env_var variable names Nov 13, 2022
@adamgreig
Copy link
Contributor Author

I've updated to accept the old case for the name alongside the NMIGEN version and the new uppercase version. If multiple variables are specified then the deprecated versions are executed first, followed by the new uppercase version, which means it should be the one to actually take effect - which is the same behaviour as before when both NMIGEN and AMARANTH versions were specified.

I've only tested the ECP5 toolchain as I don't have tools installed for the others.

@whitequark whitequark merged commit af7c114 into amaranth-lang:main Nov 16, 2022
@whitequark
Copy link
Member

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants