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.

Sign up for GitHub

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

Jump to bottom

avoid suggesting that toolchainopts are supported for SYSTEM compiler, since they're not #4585

Merged
merged 7 commits into from
Aug 27, 2024
Merged

avoid suggesting that toolchainopts are supported for SYSTEM compiler, since they're not #4585

merged 7 commits into from
Aug 27, 2024

Conversation

ocaisa
Copy link
Member

@ocaisa ocaisa commented Jul 23, 2024

Fixes #4584

@ocaisa
Copy link
Member Author

ocaisa commented Jul 23, 2024

Whether we should support toolchainopts is up for debate, but this PR reflects the current status

@ocaisa
Copy link
Member Author

ocaisa commented Jul 23, 2024

This will require a companion PR to SYSTEM easyconfigs that try to set toolchainopts as this will no longer be allowed

@branfosj
Copy link
Member

This will require a companion PR to SYSTEM easyconfigs that try to set toolchainopts as this will no longer be allowed

See easybuilders/easybuild-easyconfigs#21035 - though that targets the 5.0.x branch.

@ocaisa ocaisa changed the base branch from develop to 5.0.x July 23, 2024 15:11
@ocaisa ocaisa changed the base branch from 5.0.x to develop July 23, 2024 15:11
@ocaisa ocaisa changed the base branch from develop to 5.0.x July 23, 2024 15:19
@ocaisa
Copy link
Member Author

ocaisa commented Jul 23, 2024

@branfosj I've retargeted this to 5.0 as well

@branfosj branfosj added this to the 5.0 milestone Jul 23, 2024
@Micket
Copy link
Contributor

Micket commented Jul 23, 2024

Would it not be more appropriate to do this to SystemCompiler instead of SystemToolchain?
Does it need to set COMPILER_UNIQUE_OPTION_MAP to none as well, given that it is none from the start?

I really don't think there is any reason not to support these though, everything in COMPILER_SHARED_OPTS is expected to be shared among all the compilers, so we certainly would assume the system compiler to support them as well.

@ocaisa
Copy link
Member Author

ocaisa commented Jul 24, 2024

This is not trivial to do, as for the system compiler there are specific things happening

easybuild-framework/easybuild/tools/toolchain/toolchain.py

Lines 829 to 856 in e913a78

if self.is_system_toolchain():
# define minimal build environment when using system toolchain;
# this is mostly done to try controlling which compiler commands are being used,
# cfr. https://github.com/easybuilders/easybuild-framework/issues/3398
self.set_minimal_build_env()
else:
trace_msg("defining build environment for %s/%s toolchain" % (self.name, self.version))
if not self.dry_run:
self._verify_toolchain()
# Generate the variables to be set
self.set_variables()
# set the variables
# onlymod can be comma-separated string of variables not to be set
if onlymod is True:
self.log.debug("prepare: do not set additional variables onlymod=%s", onlymod)
self.generate_vars()
else:
self.log.debug("prepare: set additional variables onlymod=%s", onlymod)
# add LDFLAGS and CPPFLAGS from dependencies to self.vars
self._add_dependency_variables()
self.generate_vars()
self._setenv_variables(onlymod, verbose=not silent)

@branfosj
Copy link
Member

@branfosj I've retargeted this to 5.0 as well

Once easybuilders/easybuild-easyconfigs#21035 is approved and merged then I am happy to proceed with this. It makes the processing of easyconfigs behave how the SYSTEM toolchain works currently - avoiding unexepected behaviour. We can always revisit making the SYSTEM toolchain behave more similarly to other toolchains in the future.

@ocaisa
Copy link
Member Author

ocaisa commented Jul 24, 2024

@Micket I did give this a try this morning but it is complicated, I could not successfully extract the CFLAGS and CXXFLAGS. I've moved the changes to the parent class and left a comment there about what you would need to do if you wanted to change things

@ocaisa ocaisa mentioned this pull request Jul 24, 2024
Merged
@ocaisa
Copy link
Member Author

ocaisa commented Jul 25, 2024 •
edited
Loading

There is a failing test that I'm amazed worked in the first place:

easybuild-framework/test/framework/toy_build.py

Lines 2739 to 2745 in 1148625

# test use of rpath toolchain option
test_ecs = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'easyconfigs', 'test_ecs')
toy_ec_txt = read_file(os.path.join(test_ecs, 't', 'toy', 'toy-0.0.eb'))
toy_ec_txt += "\ntoolchainopts = {'rpath': False}\n"
toy_ec = os.path.join(self.test_prefix, 'toy.eb')
write_file(toy_ec, toy_ec_txt)
self.test_toy_build(ec_file=toy_ec, extra_args=['--rpath'], raise_error=True)

@ocaisa
Copy link
Member Author

ocaisa commented Jul 30, 2024

@branfosj Should be good to go now

@boegel boegel added the EasyBuild-5.0 EasyBuild 5.0 label Jul 31, 2024
@boegel boegel changed the title toolchainopts are not supported for SYSTEM compiler avoid suggesting that toolchainopts are supported for SYSTEM compiler, since they're not Jul 31, 2024
boegel
boegel previously approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

boegel
boegel approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel merged commit 131d408 into easybuilders:5.0.x Aug 27, 2024
35 checks passed
This was referenced Aug 28, 2024
Merged
Merged
@boegel boegel mentioned this pull request Oct 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boegel boegel boegel approved these changes

Assignees
No one assigned
Labels
bug fix EasyBuild-5.0 EasyBuild 5.0
Projects
EasyBuild v5.0
Status: No status
Milestone
5.0
Development

Successfully merging this pull request may close these issues.

Auto-generated documentation for SYSTEM compiler is incorrect
4 participants
@ocaisa @branfosj @Micket @boegel