Skip to content

Add test for duplicate PYTHONPATH in modextrapaths#19061

Open
Flamefire wants to merge 5 commits intoeasybuilders:developfrom
Flamefire:add-test-for-duplicate-pythonpath
Open

Add test for duplicate PYTHONPATH in modextrapaths#19061
Flamefire wants to merge 5 commits intoeasybuilders:developfrom
Flamefire:add-test-for-duplicate-pythonpath

Conversation

@Flamefire
Copy link
Copy Markdown
Contributor

@Flamefire Flamefire commented Oct 23, 2023

And similar test for sanity_check_paths.

Intended to avoid issues fixed in #13385

Edit: Changed to using regexs to catch hard-coded version numbers instead of %(pyshortver)s.

Example failure using one of the faulty ECs from #13385:

CIRCexplorer-1.1.10-intel-2017b-Python-2.7.14.eb: sanity_pip_check should be enabled
CIRCexplorer-1.1.10-intel-2017b-Python-2.7.14.eb: modextrapaths contains superflous 'lib/python2.7/site-packages' (automatically added by easyblock)
CIRCexplorer-1.1.10-intel-2017b-Python-2.7.14.eb: sanity_check_paths['dirs'] contains superflous ['lib/python2.7/site-packages'] (automatically added by easyblock)

Requires:

@Flamefire
Copy link
Copy Markdown
Contributor Author

@boegel Anything missing here? This is a CI test change only so shouldn't cause any regressions for users (but actually helps avoiding them)

@boegel boegel modified the milestones: 4.9.1, release after 4.9.1 Apr 3, 2024
@Flamefire Flamefire force-pushed the add-test-for-duplicate-pythonpath branch from d45f0b6 to aec46ee Compare April 16, 2024 10:19
@boegel boegel modified the milestones: 4.9.2, release after 4.9.2 Jun 8, 2024
@boegel boegel modified the milestones: 4.9.3, release after 4.9.3 Sep 11, 2024
@Flamefire Flamefire changed the base branch from develop to 5.0.x December 19, 2024 08:34
@Flamefire Flamefire force-pushed the add-test-for-duplicate-pythonpath branch from 1ef4ac2 to e4fbb9b Compare December 19, 2024 08:34
@Flamefire
Copy link
Copy Markdown
Contributor Author

Flamefire commented Dec 19, 2024

Retargeted 5.0x

@boegel
Copy link
Copy Markdown
Member

boegel commented Feb 5, 2025

@Flamefire Sync with 5.0.x now that #22061 is merged?

@Flamefire Flamefire force-pushed the add-test-for-duplicate-pythonpath branch from e4fbb9b to 7e4054b Compare February 5, 2025 13:32
@Flamefire
Copy link
Copy Markdown
Contributor Author

Done. Tests pass

@boegel boegel modified the milestones: release after 4.9.4, release after 5.0.0 Mar 18, 2025
@Flamefire Flamefire force-pushed the add-test-for-duplicate-pythonpath branch from f450551 to 2eb7040 Compare May 8, 2025 07:42
@Flamefire
Copy link
Copy Markdown
Contributor Author

@boegel Rebased this again and extended to checking the resolved values as well as one instance used /lib/python%s...' % local_pyshortver which will now be caught too

Can this be merged before new ECs with those paths get introduced?

@boegel
Copy link
Copy Markdown
Member

boegel commented Sep 23, 2025

@Flamefire Needs more love (again), sorry...

@Flamefire
Copy link
Copy Markdown
Contributor Author

Rebased. Trivial merge conflict after a cleanup of a condition

for d in default_dirs if d in extra_python_path)
if easyblock == 'PythonBundle' or easyblock.endswith('PythonPackage'):
sanity_check_dirs = ec.get('sanity_check_paths', dict()).get('dirs') or []
default_dirs = (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this use [ and not ( ??

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The general guideline I follow is: If the list doesn't need to be modified it will be a tuple

Not sure if this is also faster (due to less capabilities to cater for) but at least it avoids accidental changes where none are intended

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But you are modifying it in the with ec.allow... for loop, line 1376

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, not sure how I missed that as it would always fail

akesandgren
akesandgren previously approved these changes Oct 14, 2025
Copy link
Copy Markdown
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

LGTM

@akesandgren
Copy link
Copy Markdown
Contributor

@boegel Please check this out. It looks good and clean to me...

@akesandgren
Copy link
Copy Markdown
Contributor

akesandgren commented Oct 15, 2025

@Flamefire Hmm... I just caused this to have a conflict when merging 21362

Please adjust it

@Flamefire
Copy link
Copy Markdown
Contributor Author

Rebased and fixed

@Flamefire Flamefire force-pushed the add-test-for-duplicate-pythonpath branch from b8b6c2d to 9fa9549 Compare November 24, 2025 07:58
@boegel boegel modified the milestones: next release (5.2.1), 5.x Feb 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants