Skip to content
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.

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

normalize mtime #1839

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Conversation

katexochen
Copy link
Contributor

First part of the implementation of #687

If set, the time stamp from SOURCE_DATE_EPOCH is used to normalize mtime of files. We also need to pass the environment trough when mkosi is invoking itself.

@katexochen katexochen force-pushed the feat/root-mtime-source-epoch branch from f98a142 to 8c54e20 Compare August 25, 2023 19:21
mkosi/archive.py Outdated Show resolved Hide resolved
mkosi/archive.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
@katexochen katexochen force-pushed the feat/root-mtime-source-epoch branch from 8c54e20 to 543459f Compare August 26, 2023 13:58
mkosi/archive.py Outdated Show resolved Hide resolved
@katexochen katexochen force-pushed the feat/root-mtime-source-epoch branch from 543459f to f716ab3 Compare August 26, 2023 15:09
Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

LGTM.

mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/archive.py Outdated Show resolved Hide resolved
@keszybz
Copy link
Member

keszybz commented Aug 27, 2023

Oh, and please put the text that is now the description of the pull request in the commit message. Signed-off-by is not defined by mkosi, so that can be dropped.

@katexochen katexochen force-pushed the feat/root-mtime-source-epoch branch from f716ab3 to 053dd1f Compare August 27, 2023 17:22
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

We should normalize the mtime before calling make_tar() or make_cpio() instead of doing it inside those functions

@malt3
Copy link
Contributor

malt3 commented Aug 28, 2023

We should normalize the mtime before calling make_tar() or make_cpio() instead of doing it inside those functions

The reason we did it this way is that there is a high probability that the code becomes incorrect over time otherwise.

We noticed that we need to normalize mtimes multiple times during a build since cpios for example are created during the build (and need normalized mtimes), after which files in the sysroot are modified (requiring us to normalize mtimes again).
If we place this code in make_tar, make_cpio and make_image, there is no chance for file modifications between normalizing mtimes and creating the archive / partitions.

@katexochen katexochen force-pushed the feat/root-mtime-source-epoch branch from 053dd1f to 070a1fb Compare August 29, 2023 07:48
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/archive.py Outdated Show resolved Hide resolved
@katexochen katexochen force-pushed the feat/root-mtime-source-epoch branch from 070a1fb to 73eb7e5 Compare August 29, 2023 10:38
mkosi/state.py Outdated Show resolved Hide resolved
mkosi/state.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
@katexochen katexochen force-pushed the feat/root-mtime-source-epoch branch 2 times, most recently from 133b3ff to b77c548 Compare August 29, 2023 15:28
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/state.py Outdated Show resolved Hide resolved
mkosi/state.py Outdated Show resolved Hide resolved
mkosi/state.py Outdated Show resolved Hide resolved
mkosi/state.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
@katexochen katexochen force-pushed the feat/root-mtime-source-epoch branch from 9f73009 to 0b1def6 Compare August 30, 2023 09:57
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

Hmm this seems to granular, we should just normalize the mtimes of /boot and /efi again before we call make_image() again instead of doing it for each invididual file

@katexochen katexochen force-pushed the feat/root-mtime-source-epoch branch from 0b1def6 to 2b27ec4 Compare August 30, 2023 12:13
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

Ah and maybe also modify load_environment() to pick up SOURCE_DATE_EPOCH from the environment mkosi is invoked from

@katexochen katexochen force-pushed the feat/root-mtime-source-epoch branch from 2b27ec4 to 2fea405 Compare August 30, 2023 12:57
mkosi/__init__.py Outdated Show resolved Hide resolved
@katexochen katexochen force-pushed the feat/root-mtime-source-epoch branch from 2fea405 to e7b5486 Compare August 30, 2023 13:17
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

Sorry for the many rounds, but I realized we should make this a separate setting so it doesn't have to be configured explicitly via the environment. So what I would suggest is the following:

  1. Introduce a new setting SourceDateEpoch, it's parse function should be more or less identical to the logic you have in the MkosiState constructor now.
  2. Add a default_factory function for SourceDateEpoch that initializes it from the configured environment (Environment=) or the host environment (in that order).
  3. Instead of passing state.source_date_epoch to normalize_mtime(), pass state.config.source_date_epoch to normalize_mtime().

@malt3 malt3 force-pushed the feat/root-mtime-source-epoch branch from e7b5486 to 1e76abf Compare August 30, 2023 14:56
@malt3 malt3 force-pushed the feat/root-mtime-source-epoch branch from 1e76abf to 2f47bc9 Compare August 30, 2023 14:59
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

Almost there

mkosi/config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/resources/mkosi.md Outdated Show resolved Hide resolved
mkosi/resources/mkosi.md Outdated Show resolved Hide resolved
@malt3 malt3 force-pushed the feat/root-mtime-source-epoch branch 2 times, most recently from 47b0860 to d07892c Compare August 30, 2023 16:06
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
@malt3 malt3 force-pushed the feat/root-mtime-source-epoch branch 2 times, most recently from 44683fd to 78bc91d Compare August 31, 2023 08:09
Copy link
Contributor

@DaanDeMeyer DaanDeMeyer left a comment

Choose a reason for hiding this comment

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

Found one more thing

mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
@malt3 malt3 force-pushed the feat/root-mtime-source-epoch branch from 78bc91d to 46582ce Compare August 31, 2023 08:15
@malt3
Copy link
Contributor

malt3 commented Aug 31, 2023

Stop. Do not merge. I'm quite sure I found a missing int -> str conversion issue. I'll perform some more tests.

EDIT: Alright. Works as expected now. Sorry for that.

If set, the time stamp from SOURCE_DATE_EPOCH is used to normalize
mtime of files. We also need to pass the environment trough when
mkosi is invoking itself.

Co-authored-by: Malte Poll <mp@edgeless.systems>
@malt3 malt3 force-pushed the feat/root-mtime-source-epoch branch from 46582ce to 4ccad9e Compare August 31, 2023 08:38
@DaanDeMeyer
Copy link
Contributor

Thanks for contributing!

@DaanDeMeyer DaanDeMeyer merged commit bdf37e1 into systemd:main Aug 31, 2023
@katexochen katexochen deleted the feat/root-mtime-source-epoch branch August 31, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants