Skip to content

Keep list of patches unique before attempting a copy#4959

Closed
ocaisa wants to merge 1 commit intoeasybuilders:developfrom
ocaisa:copy_file_bug
Closed

Keep list of patches unique before attempting a copy#4959
ocaisa wants to merge 1 commit intoeasybuilders:developfrom
ocaisa:copy_file_bug

Conversation

@ocaisa
Copy link
Copy Markdown
Member

@ocaisa ocaisa commented Jul 10, 2025

It was noted that for patches of extensions you can end up with multiple entries for a patch in the final list of patches. This change tries to make sure that each entry only appears once.

This actually can lead to a bug (observed when using EESSI) as the copy will fail the second time around when the target file is not writable (it was read only on the initial copy) and came from a filesystem with extended attributes (like /cvmfs).

copy_file(patch['path'], target)
_log.debug("Copied patch %s to %s", patch['path'], target)

application_log = os.path.join(new_log_dir, log_fn)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

log messages when trying to copy over files were disappearing into the abyss as the logs were already closed at this point. Moved the log copy to the last possible location

@bedroge
Copy link
Copy Markdown
Contributor

bedroge commented Jul 11, 2025

I debugged this a bit more, as I was curious why the patches showed up in that list twice. By adding some print statements throughout the code, I found that it's because of patches = app.patches which I used in #4939. This doesn't actually copy the list, and as it then is going to add the extension patches to patches, they will also show up in app.patches. This piece of code is run twice - once for copying them to the repo, and once for copying them to the reprod dir - causing duplicate entries to appear in app.patches.

Instead of making sure that there are no duplicates, I think we should prevent that they show up in app.patches at all, as that may have unintended side effect. I'll make a PR that prevents that from happening (by making an actual copy of the list), which should also solve this issue.

@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Jul 11, 2025

Prefer the approach in #4960

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.

3 participants