Skip to content

store all patches (incl the ones for extensions) that need to be copied to a repo/reprod dir in an all_patches set#4960

Merged
ocaisa merged 5 commits intoeasybuilders:developfrom
bedroge:copy_patches_fix
Jul 11, 2025
Merged

store all patches (incl the ones for extensions) that need to be copied to a repo/reprod dir in an all_patches set#4960
ocaisa merged 5 commits intoeasybuilders:developfrom
bedroge:copy_patches_fix

Conversation

@bedroge
Copy link
Copy Markdown
Contributor

@bedroge bedroge commented Jul 11, 2025

This also solves the duplication issue that #4959 tried to solve, which may cause issues for anyone trying to install an easyconfig that has an extension with a patch, and when that patch has read-only file permissions (this may happen frequently if EasyBuild itself was installed with --read-only-installdir).

Note that patches of components are added to self.patches by the Bundle easyblock: https://github.com/easybuilders/easybuild-easyblocks/blob/develop/easybuild/easyblocks/generic/bundle.py#L222. Hence, they will also be included in self.all_patches.
Do note that, regardless of the change in this PR, the approach used by the Bundle may actually cause issues, because sources/patches may then be expected by the main easyblock itself, and they may also end up in the easyconfig stored in the reprod folder (see #4862). However, I felt that solving that was out of scope for this PR, and could be handled in a follow-up PR.

edit: discussed this a bit more with @ocaisa on Slack, and converted all_patches into a set containing FrozenDicts. This prevents that duplicate entries may show up in the list, i.e. it would prevent that due to future code changes the same issue as #4959 would pop up again. When I tried to use FrozenDicts, I got this error:

    self.__hash = reduce(operator.xor, map(hash, self.iteritems()), 0)
                                                 ^^^^^^^^^^^^^^
AttributeError: 'FrozenDict' object has no attribute 'iteritems'

Solved this by changing self.iteritems() to self.items().

edit2: we actually only need to know the paths to all patches, so let's only store those instead of the entire patch_info dictionary.

ocaisa
ocaisa previously approved these changes Jul 11, 2025
Copy link
Copy Markdown
Member

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

This looks very clean now, if the existing tests (https://github.com/easybuilders/easybuild-framework/blob/develop/test/framework/toy_build.py#L1390) pass I think this is good to go

@bedroge bedroge changed the title add all_patches variable to store all patches (incl the ones for extensions) that need to be copied to a repo/reprod dir store all patches (incl the ones for extensions) that need to be copied to a repo/reprod dir in a all_patches set Jul 11, 2025
@bedroge bedroge changed the title store all patches (incl the ones for extensions) that need to be copied to a repo/reprod dir in a all_patches set store all patches (incl the ones for extensions) that need to be copied to a repo/reprod dir in an all_patches set Jul 11, 2025
@ocaisa ocaisa enabled auto-merge July 11, 2025 11:32
Copy link
Copy Markdown
Member

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

LGTM

@ocaisa ocaisa merged commit f4e855b into easybuilders:develop Jul 11, 2025
46 of 47 checks passed
@bedroge bedroge deleted the copy_patches_fix branch July 11, 2025 13:02
@boegel boegel added the bug fix label Jul 30, 2025
@boegel boegel added this to the release after 5.1.1 milestone Jul 30, 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.

3 participants