fix logic in Cargo easyblock to determine value for 'finalpath' after extracting sources#4022
Conversation
… extracting sources
|
@boegelbot please test @ jsc-zen3 |
|
@boegel: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... Details- notification for comment with ID 3652149772 processed Message to humans: this is just bookkeeping information for me, |
|
|
||
| self.log.info("Unpacking source of %s", src['name']) | ||
| existing_dirs = set(os.listdir(extraction_dir)) | ||
| existing_files = set(os.listdir(extraction_dir)) |
There was a problem hiding this comment.
Can we not name this "files"? That would be as wrong as "dirs", so maybe "paths"?
There was a problem hiding this comment.
You're right, but not going to change this now, since it's a local variable, and if we're strict that would require another round of testing...
Please open a follow-up PR (or squeeze the renaming of the local variable in another PR for Cargo easyblock)
| extra_options=self.cfg['unpack_options'], change_into_dir=False, trace=False) | ||
| new_extracted_dirs = set(os.listdir(extraction_dir)) - existing_dirs | ||
| new_extracted_files = set(os.listdir(extraction_dir)) - existing_files | ||
| new_extracted_dirs = sorted(x for x in new_extracted_files |
There was a problem hiding this comment.
This introduces a change to previous behavior which used the result of extract_file which has the same logic as this had before: If extraction results in multiple files and/or directories the parent/extractiondir is returned.
This is also problematic, see easybuilders/easybuild-framework#4922
My tests included that PR so I added the change to #3665 to not rely on the (changed by 4922) result but misunderstood the (questionable) logic.
There was a problem hiding this comment.
This is essentially fixing a regression that was introduced in #3665, so it should be merged.
The installation of helix-25.07.1-GCCcore-14.3.0.eb is broken without this fix, while it worked fine before #3665 got merged.
I'm testing this very broadly (see list of easyconfig files passed to the bot) deliberately to make sure this doesn't break any other easyconfigs we have currently.
I consider this a (small) blocker for the release of EasyBuild v5.2.0.
If more steps are needed afterwards to make things more sensible, that's OK, but without this fix the logic used to determine path to set in finalpath is just flawed: it should never be the path to a file (but a directory), and it should also not be a random path, as is happening now (it should always be the same path, even if that ends up being the wrong path).
So, this is a step forward, but probably not the end of the ride.
There was a problem hiding this comment.
Yes, but it does not restore the behavior prior to #3665 if this is what the intention was. The prior behavior included files when checking which folder to use, see extract_file in framework: It returns the first folder that contains more than a single folder, which in this case is the parent folder.
The fix here only happens to work because there are multiple folders. But it would still trigger the regression if helix contained a single folder and and single file.
The previous logic was:
src_dir = extract_file(..)-> Returnsextraction_dirforhelixif len(new_extracted_dirs) == 1: src_dir = os.path.join(extraction_dir, new_extracted_dirs.pop())if is_vendor_crate and self.cfg['offline'] and git_key and member_dirs is None: src_dir = os.path.join(self.vendor_dir, os.path.basename(crate_dirs[0]))
src['finalpath'] = src_dir
There was a problem hiding this comment.
Maybe it won't work in a theoretical case, but if it works in practice for all easyconfigs we have, it should still go in.
We can follow up in a subsequent PR with more fixes if needed.
There was a problem hiding this comment.
I just wanted to make sure you are aware that the added line, filtering for directories, is a breaking change, similar to what happened in #3665 and introduces a difference in folder-detection to a) extract_file in framework b) the likely intended behavior in easybuilders/easybuild-framework#4922 and c) EB 5.1 Cargo
Although it is at least (more) deterministic than after #3665 so I'm fine with that if it works for the tested ECs
|
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 126 out of 126 (total: 18 hours 51 mins 24 secs) (120 easyconfigs in total) |
(created using
eb --new-pr)fixes #4021