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

Test suite does not accept SYSTEM as valid toolchain for dep #15901

Closed
ocaisa opened this issue Jul 22, 2022 · 8 comments
Closed

Test suite does not accept SYSTEM as valid toolchain for dep #15901

ocaisa opened this issue Jul 22, 2022 · 8 comments

Comments

@ocaisa
Copy link
Member

ocaisa commented Jul 22, 2022

See https://github.com/easybuilders/easybuild-easyconfigs/runs/7471210039?check_suite_focus=true#step:8:160

@Micket
Copy link
Contributor

Micket commented Jul 29, 2022

I don't understand. Do you have an example easyconfig of what should work but doesnt?

@ocaisa
Copy link
Member Author

ocaisa commented Jul 29, 2022

I did, but to get the PR to pass the tests I changed it :P see 494fc0e

@Micket
Copy link
Contributor

Micket commented Jul 29, 2022

Right though SYSTEM is a macro thing that is expanded to the toolchain dict. We surely can't put that there
I can imagine changing this issue to the (valid) complaint:

Using

    ('CUDA', '11.4.1', '', True),

to denote system toolchain is cryptic.

Maybe we can have some other appropriately named macro that just evaluates to True there?

@ocaisa
Copy link
Member Author

ocaisa commented Aug 3, 2022

A fix for this is now part of #15900

@Micket
Copy link
Contributor

Micket commented Aug 3, 2022

Discussed during the zoom meeting;
I hadn't realized that using the full toolchain dict just actually worked there, i thought it had to be True, so that completely negates everything I've said in this issue so far.

@boegel mentioned we might need to investigate the behaviour of dumpenv to see if we perhaps need to fix something in framework.

Otherwise, we were all very much in favour of doing this everywhere going forward.

@boegel boegel added this to the 4.x milestone Aug 4, 2022
@boegel
Copy link
Member

boegel commented Aug 4, 2022

Indeed, using SYSTEM rather than True is way more intuitive, so going forward with that definitely makes sense.

@ocaisa The workaround you had to do in the easyconfigs suite is probably a signal that we should change something in framework instead, i.e. avoid that True is being used when an easyconfig is being dumped, and use the SYSTEM constant instead?
Then there should be no need for the workaround you implemented in #15900...

@Micket
Copy link
Contributor

Micket commented Aug 10, 2022

i.e. avoid that True is being used when an easyconfig is being dumped, and use the SYSTEM constant instead?

whatever we pick, it'll have to resolve to something right? So any dump checker will have to account for the implied equality between True and {'system': ....} unless we really want to make one of those an error?

@Micket
Copy link
Contributor

Micket commented Apr 2, 2024

This is definitely solved (in fact, the test suite insists on this now)

@Micket Micket closed this as completed Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants