Skip to content

Do not fail for read-only installation directory on --module-only (re)builds#4958

Merged
boegel merged 2 commits intoeasybuilders:developfrom
Flamefire:readonly-installdir-module-rebuild
Aug 9, 2025
Merged

Do not fail for read-only installation directory on --module-only (re)builds#4958
boegel merged 2 commits intoeasybuilders:developfrom
Flamefire:readonly-installdir-module-rebuild

Conversation

@Flamefire
Copy link
Copy Markdown
Contributor

When only creating a module the installation directory must not be touched as it might be on a read-only file system.

Skip the code that copies reprod-files (easyconfig, easyblock, hooks) and log files.

This came up when trying to create a module-tree for EESSI reusing the installed software.

When only creating a module the installation directory must not be
touched as it might be on a read-only file system.

Skip the code that copies reprod-files (easyconfig, easyblock, hooks)
and log files.
@Flamefire Flamefire force-pushed the readonly-installdir-module-rebuild branch from 3b1c6cc to e312f26 Compare July 30, 2025 08:32
@boegel boegel added this to the release after 5.1.1 milestone Jul 30, 2025
@boegel
Copy link
Copy Markdown
Member

boegel commented Jul 30, 2025

While this makes sense, it does imply that we'll no longer have an extra log file in the installation directory for the --module-only run that caused the module file to be re-generated.

Rather than not copying the log file, can we make sure we can copy by temporarily enabling write permissions on the installation directory, and print a warning to make it clear that the log file can't be copied if that didn't work?

@Flamefire
Copy link
Copy Markdown
Contributor Author

Rather than not copying the log file, can we make sure we can copy by temporarily enabling write permissions on the installation directory, and print a warning to make it clear that the log file can't be copied if that didn't work?

We actually do that already. Just if the change didn't work (e.g. on EESSI) it still fails.
I can try catching all places where it might fail and show a warning instead.

@boegel
Copy link
Copy Markdown
Member

boegel commented Jul 30, 2025

Just producing a clear warning (when using --module-only) makes sense, I think

@Flamefire
Copy link
Copy Markdown
Contributor Author

Ok, I refactored the code a bit to catch errors early to provide an exact warning. It now looks like this:

== creating module...
== comparing module file with backup /tmp/foo/all/Core/GCCcore/12.3.0.bak_20250731123417_3413057; no differences found
== ... (took 1 secs)
== permissions [skipped]
== packaging [skipped]

WARNING: Cannot make directory /cvmfs/software.eessi.io/versions/2023.06/software/linux/x86_64/amd/zen4/software/GCCcore/12.3.0/easybuild writeable.
So logs, easyconfigs, patches and files for reproducibility will not be saved.

== COMPLETED: Installation ended successfully (took 19 secs)
== Results of the build can be found in the log file(s) /tmp/easybuild-tmplog/easybuild-GCCcore-12.3.0-20250731.123417.qTtOk.log

== Build succeeded for 1 out of 1
== Summary:
   * [SUCCESS] Core/GCCcore/12.3.0
== Temporary log file(s) /tmp/easybuild-tmplog/easybuild-uzx7mqhu.log* have been removed.
== Temporary directory /tmp/easybuild-tmp/eb-26biil95 has been removed.

I found that not all logs get deleted even on success. Hence I also assigned app.logfile to application_log so its path is shown for inspection when it couldn't be moved like in this case.

@Flamefire Flamefire changed the title Do not touch installation directory for --module-only (re)builds Do not fail for read-only installation directory on --module-only (re)builds Aug 5, 2025
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel merged commit e062d77 into easybuilders:develop Aug 9, 2025
37 checks passed
@Flamefire Flamefire deleted the readonly-installdir-module-rebuild branch August 10, 2025 13:49
change_dir(cwd)

application_log = None
application_log = app.logfile
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was a bit too naive, but it was easy to overlook that it could have consequences.

It introduced a regression, which was fixed in time for the EasyBuild v5.1.2 release, see:

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.

Thinking about how this happens. Previously application_log assignment was guarded by if result and not dry_run: with a fallback if not result otherwise it would never be assigned.

So I guess the extended-dry-run does set app.logfile (to what value actually and why?) and always returns success (result = True)

I'm wondering if extended-dry-run sets app.logfile to something that looks valid if we shouldn't handle it similar to other code that similarly would need to check that. Just to be sure we are not missing something similar somewhere else.

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.

2 participants