Skip to content

use source toolchain version when passing only --try-toolchain#4395

Merged
ocaisa merged 2 commits intoeasybuilders:developfrom
Flamefire:fix-try-toolchain
Dec 5, 2023
Merged

use source toolchain version when passing only --try-toolchain#4395
ocaisa merged 2 commits intoeasybuilders:developfrom
Flamefire:fix-try-toolchain

Conversation

@Flamefire
Copy link
Copy Markdown
Contributor

process_software_build_specs always adds a toolchain key even when only the name of the toolchain is available. This makes the code in tweak ignore the code path in using toolchain_name and/or toolchain_version with the fallback to using the values of the source toolchain.
This in turn leads to failures further down when a value of None is used where an actual version is expected.

Fix by only adding the toolchain key when we have both the name and version and hence a complete, valid toolchain spec.

CC @boegel who added the code using a default of None for the version in #810 Maybe you remember the reasoning?

`process_software_build_specs` ialways adds a `toolchain` key
even when only the name of the toolchain is available.
This makes the code in `tweak` ignore the code path in using
`toolchain_name` and/or `toolchain_version` with the fallback to using
the values of the source toolchain.
This in turn leads to failures further down when a value of `None` is
used where an actual version is expected.

Fix by only adding the `toolchain` key when we have both the name and
version and hence a complete, valid toolchain spec.
Copy link
Copy Markdown
Member

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

LGTM

@ocaisa ocaisa added the bug fix label Dec 5, 2023
@ocaisa ocaisa added this to the next release (4.9.0?) milestone Dec 5, 2023
@ocaisa ocaisa merged commit badc27c into easybuilders:develop Dec 5, 2023
@Flamefire Flamefire deleted the fix-try-toolchain branch December 5, 2023 14:14
@easybuilders easybuilders deleted a comment from boegelbot Dec 6, 2023
@boegel
Copy link
Copy Markdown
Member

boegel commented Dec 6, 2023

CC @boegel who added the code using a default of None for the version in #810 Maybe you remember the reasoning?

Not sure what the reasoning was here, doesn't make much sense indeed, your fix does make sense

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants