Skip to content

Avoid processing the same EasyConfig multiple times #4767

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

Merged
merged 1 commit into from
Mar 16, 2025

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Feb 19, 2025

When passing the same EasyConfig file multiple times on the commandline it will be parsed once and put into a cache.
Every future call will get the same EasyConfig instance so any modification to it will be reflected in all future uses of supposedly "freshly parsed EasyConfigs".
This leads to confusing failures when trying to build the same file twice which is likely a mistake anyway, especially when one file is a symlink to another one passed too.
So just make sure each physical file is parsed only once.

I ran into this when building a software using usempi: True that errored during configure due to this:

== INFO Environment variable CXX set to mpicxx mpicxx (previously undefined)
== INFO Environment variable EBVARCXX set to mpicxx mpicxx (previously undefined)
== INFO Environment variable CXXFLAGS set to -O3 -march=native -fno-math-errno -g -O3 -march=native -O3 -march=native -fno-math-errno -fno-math-errno -g (previously undefined)

This is actually a bug introduced by 3836140:

  • previously copy.deepcopy was used which would copy the easyconfig instances contained in the cached dict
  • Now only a shallow copy of the dict is made, i.e. there is a separate instance of the dict but not of the keys and values

I don't think this was intentional, the commit message reads

use copy() method rather than copy.deepcopy on EasyConfig instances

But the operation is done on dict instances not EasyConfig instances. Shall we fix that too @boegel ?
That would we an alternative to this PR: If we do get independent EC instances then building the same EC twice should work. But I'd prefer this or both.

When passing the same EasyConfig file multiple times on the commandline
it will be parsed once and put into a cache.
Every future call will get the same `EasyConfig` instance so any
modification to it will be reflected in all future uses of supposedly
"freshly parsed EasyConfigs".
This leads to confusing failures when trying to build the same file twice
which is likely a mistake anyway, especially when one file is a symlink
to another one passed too.
So just make sure each physical file is parsed only once.
@boegel boegel added the bug fix label Feb 24, 2025
@boegel boegel added this to the release after 4.9.4 milestone Feb 24, 2025
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 b1f5cc9 into easybuilders:develop Mar 16, 2025
37 checks passed
@Flamefire
Copy link
Contributor Author

Flamefire commented Mar 17, 2025

Shall we fix the copy issue too? I.e. again use copy.deepcopy not (shallow) dict.copy? #4818

@Flamefire Flamefire deleted the duplicate-ec-avoid branch March 17, 2025 07:57
@boegel boegel modified the milestones: release after 4.9.4, release after 5.0.0, 5.0.0 Mar 17, 2025
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.

2 participants