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

Update HierarchicalMNS for GCCcore toolchain #2870

Merged
merged 4 commits into from May 14, 2019

Conversation

ocaisa
Copy link
Member

@ocaisa ocaisa commented May 9, 2019

@Micket With our custom naming scheme (related to HMNS) I was also seeing an issue with the Clang easyconfig in easybuilders/easybuild-easyconfigs#8254 This PR fixes that for me, however the reported error (for me) was:

File "/tmp/eb-KByFqx/included-module-naming-schemes/easybuild/tools/module_naming_scheme/custom_hierarchical_mns.py", line 143, in det_modpath_extensions
    comp_name_ver = [comp_name, comp_ver_tmpl % comp_versions]
KeyError: 'GCC'

This is quite different from what you reported in
easybuilders/easybuild-easyconfigs#8254 (comment)
Does this PR fix your problem?

@ocaisa
Copy link
Member Author

ocaisa commented May 9, 2019

Ok, I reproduced the error that was seen for @Micket , this fixes it.

@boegel boegel added this to the next release (3.9.1) milestone May 9, 2019
@boegel boegel added the bug fix label May 9, 2019
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, but I'd like to get confirmation from @Micket on this before merging...

@boegel boegel changed the title Update hmns for GCCcore Update HierarchicalMNS for GCCcore toolchain May 9, 2019
@akesandgren
Copy link
Contributor

This change can perhaps also make it possible to combine CUDA with GCCcore instead of GCC to make the GCC-CUDA based stuff.... might be worth looking at in the future...

@@ -189,6 +192,9 @@ def det_modpath_extensions(self, ec):

if non_dummy_tc:
tc_comp_name, tc_comp_ver = tc_comp_info
# Stick to name GCC for GCCcore
if tc_comp_name == GCCCORE:
tc_comp_name = 'GCC'
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this won't cause things built with GCCcore to end up in .../Compiler/GCC instead of Compiler/GCCcore

I'm not sure exactly what this affects.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it won't affect that since GCCcore is built with the dummy compiler (so the condition non_dummy_tc 4 lines up is not satisfied). This is only relevant to compilers built with GCCcore and only affects the searching for matches in COMP_NAME_VERSION_TEMPLATES (see the for loop around this block).

I'm not overly confident that this has no impact though but I don't believe it does. As far as I know Clang is the only compiler build from source using GCCcore (rather than GCC).

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tried this out with Clang, and then with a normal package
eb Ghostscript-9.27-GCCcore-8.2.0.eb --include-module-naming-schemes hierarchical_mns.py
and it was placed correctly (under software/Compiler/GCCcore/Ghostscript )

@Micket
Copy link
Contributor

Micket commented May 14, 2019

lgtm, but I'd like to get confirmation from @Micket on this before merging...

@boegel
Building clang (on centos7) with this worked. I can't say I understand this part of the code, but, I have no complaints.

@boegel
Copy link
Member

boegel commented May 14, 2019

Thanks for confirming the fix @Micket!

@boegel boegel merged commit 8c1055e into easybuilders:develop May 14, 2019
@ocaisa ocaisa deleted the update_hmns_for_GCCcore branch May 14, 2019 12:40
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.

None yet

4 participants