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

Add dashes directly in compiler optimization flags #4698

Open
wants to merge 25 commits into
base: 5.0.x
Choose a base branch
from

Conversation

Micket
Copy link
Contributor

@Micket Micket commented Nov 7, 2024

Fixes #4269

It was extremely difficult to read this code, partly because a bunch of variables like:

COMPILER_F_UNIQUE_FLAGS = []

were, in fact, not flags, but options. I renamed these variables because it was so misleading. To add to this, we also have two COMPILER_FLAGS used for different things (neither of them are flags)

@Micket Micket added the EasyBuild-5.0 EasyBuild 5.0 label Nov 7, 2024
@Micket Micket added this to the 5.0 milestone Nov 7, 2024
@Micket Micket force-pushed the optarchdash branch 2 times, most recently from 35d3b7b to 24cfdc6 Compare November 8, 2024 15:03
@Micket Micket added the change label Nov 8, 2024
@Micket
Copy link
Contributor Author

Micket commented Nov 12, 2024

I finally found the place that actually adds the extra -

It boils down to using FlagList, and possible CommandFlagList.
Instead, these are not just plain StrList

I need to find a scenario where CommandFlagList was used withmore than a single argument, but it's all so very deeply hidden and obscured it's impossible to tell where the heck these are coming from. Tests indicate it works like

        cmd = CommandFlagList(["gcc", "bar", "baz"])
        self.assertEqual(str(cmd), "gcc -bar -baz")

so i need to find a scenario where this actually used, to ensure i did fix it correctly.

@Micket Micket marked this pull request as ready for review November 12, 2024 13:10
@Micket Micket requested a review from bartoldeman November 12, 2024 13:10
@Micket Micket force-pushed the optarchdash branch 3 times, most recently from 614757a to f9fba39 Compare November 12, 2024 17:54
}

COMPILER_CC = 'clang'
COMPILER_CXX = 'clang++'
COMPILER_C_UNIQUE_FLAGS = []
COMPILER_C_UNIQUE_OPTIONS = []
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, do we really need to change this?

We have to take into account that people may have their own/custom implementations of classes that derive from Compiler, they will need to be adjusted accordingly (and we have no easy way to produce a clean error message there, I think)...

easybuild/tools/toolchain/compiler.py Outdated Show resolved Hide resolved
test/framework/toolchain.py Outdated Show resolved Hide resolved
@@ -3144,21 +3145,6 @@ def test_env_vars_external_module(self):
expected = {}
self.assertEqual(res, expected)

def test_get_flag(self):
Copy link
Member

Choose a reason for hiding this comment

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

Why is this test removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can keep the test? It's a simpler function of course but can still be tested.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense, I consider get_flag to be part of the public API

easybuild/tools/toolchain/compiler.py Show resolved Hide resolved
@Micket Micket marked this pull request as draft November 13, 2024 11:44
@Micket Micket marked this pull request as ready for review November 15, 2024 16:03
@bartoldeman
Copy link
Contributor

@Micket there's a conflict now, and outstanding review comments from @boegel

Comment on lines +244 to +252
old_var = getattr(self, f'COMPILER{variant}_FLAGS', None)
if old_var is not None:
self.log.deprecated(f'COMPILER{variant}_FLAGS has been renamed to COMPILER{variant}_OPTIONS.', '6.0')
setattr(self, f'COMPILER{variant}_OPTIONS', old_var)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added warnings and backwards compat to handle the renamed variables to address @boegel review

@Micket Micket force-pushed the optarchdash branch 2 times, most recently from 13aa8c8 to 4d83479 Compare December 13, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Changed default
Development

Successfully merging this pull request may close these issues.

3 participants