Skip to content

fix logic in Cargo easyblock to determine value for 'finalpath' after extracting sources #4022

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.

Sign up for GitHub

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
bedroge merged 2 commits into from
Dec 15, 2025
+10 −8
Merged

fix logic in Cargo easyblock to determine value for 'finalpath' after extracting sources #4022

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
510220e
fix logic in Cargo easyblock to determine value for 'finalpath' after…
boegel Dec 14, 2025
707fddc
fix long line in Cargo.extract_step
boegel Dec 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions easybuild/easyblocks/generic/cargo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,21 +319,23 @@ def extract_step(self):
extraction_dir = self.vendor_dir if is_vendor_crate else self.builddir

self.log.info("Unpacking source of %s", src['name'])
existing_dirs = set(os.listdir(extraction_dir))
existing_files = set(os.listdir(extraction_dir))
Copy link
Contributor

@Flamefire Flamefire Dec 15, 2025

Choose a reason for hiding this comment

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

Can we not name this "files"? That would be as wrong as "dirs", so maybe "paths"?

Copy link
Member Author

@boegel boegel Dec 15, 2025

Choose a reason for hiding this comment

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

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)

extract_file(src['path'], extraction_dir, cmd=src['cmd'],
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
Copy link
Contributor

@Flamefire Flamefire Dec 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@boegel boegel Dec 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Flamefire Flamefire Dec 15, 2025

Choose a reason for hiding this comment

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

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(..) -> Returns extraction_dir for helix
  • if 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

Copy link
Member Author

@boegel boegel Dec 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Flamefire Flamefire Dec 15, 2025

Choose a reason for hiding this comment

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

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

if os.path.isdir(os.path.join(extraction_dir, x)))
self.log.info(f"New directories found after extracting {src['name']}: {new_extracted_dirs}")

if len(new_extracted_dirs) == 0:
# Extraction went wrong
raise EasyBuildError("Unpacking sources of '%s' failed", src['name'])
# There can be multiple folders but we just use the first new one as the finalpath
if len(new_extracted_dirs) > 1:
self.log.warning(f"Found multiple folders when extracting {src['name']}: "
f"{', '.join(new_extracted_dirs)}.")
src_dir = os.path.join(extraction_dir, new_extracted_dirs.pop())
elif len(new_extracted_dirs) == 1:
src_dir = os.path.join(extraction_dir, new_extracted_dirs[0])
else:
# if there are multiple subdirectories, we use parent directory as finalpath
src_dir = extraction_dir
self.log.debug("Unpacked sources of %s into: %s", src['name'], src_dir)

src['finalpath'] = src_dir

if self.cfg['offline']:
Expand Down