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

New handler to set mtime embedded in pyc file to 0 #27

Merged
merged 5 commits into from
Jul 18, 2024
Merged

Conversation

keszybz
Copy link
Owner

@keszybz keszybz commented Jul 17, 2024

No description provided.

prestist and others added 5 commits July 17, 2024 14:43
This is in preparation for future changes. The code for 'struct Pyc'
is put together in one place, separate from the more general code for
PycParser.

parser.clear_unused_flag_refs() modifies parser in place. A scratch
copy of the data is still used, so that modifications can be discarder
on failure. Returning a new copy of data would be awkward if we want
to do further processing on that data.
Pyc files store the mtime, size, and optionally hash of the original
py file. Mtime is used to validate the cached bytecode: the mtime
of the .py file (if present) must be equal to the stored value in
the .pyc file. If not, the pyc file is considered stale.

On ostree systems, all mtimes are set to 0. The problem is that the
.py file is created first, then the byte compilation step creates
.pyc with some embedded timestamp, and later ostree discards mtimes
on all files. On the end system, the bytecode cache is considered
invalid.

This new handler does the two very simple things:
1. zero out the mtime in the .pyc file
2. set mtime (in the filesystem) to zero for the .py file

This second step is also done by ostree, so strictly speaking, it's
not necessary. But it's easy to do it, and this way the bytecode
still matches the file on disk, so if we called Python after this
operation, it would be able to use the bytecode and wouldn't try to
rewrite it.

Replaces #26.
See ostreedev/ostree#1469,
https://gitlab.com/fedora/bootc/tracker/-/issues/3,
https://pagure.io/workstation-ostree-config/pull-request/505.
Usage: add-determinism --handler pyc-zero-mtime /some/path
@prestist
Copy link
Contributor

Added a comment on the other pr, and closed it, wanted to share it here because i know how messages/notifications can get lost. #26 (comment)

Copy link
Contributor

@lukewarmtemp lukewarmtemp left a comment

Choose a reason for hiding this comment

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

Generally LGTM

@keszybz
Copy link
Owner Author

keszybz commented Jul 18, 2024

consume the project via rust rather then cli

This is something that I haven't considered so far… The API is not designed for external consumption, it currently exposes all kinds of implementation details that are in flux as new functionality is added. I have never implemented a rust library…

I guess it could be useful to have a top-level entry point like that'd take

  • a list of filesystem paths
  • a handler list, probably with a list of string names similarly to the command-line option
  • config struct with some other details like $SOURCE_DATE_EPOCH

This could be done, but I think it'd make the most sense to do it in tandem with a caller on your side, so that we don't end up with an API that is akward to use.

@keszybz keszybz merged commit f02368f into main Jul 18, 2024
8 checks passed
@keszybz keszybz deleted the pyc-zero-mtime branch July 18, 2024 09:01
@travier
Copy link
Contributor

travier commented Jul 18, 2024

That looks great as well. Thanks

@travier
Copy link
Contributor

travier commented Jul 18, 2024

Once we have that in a release in Fedora, we can give it a try in the Atomic Desktops, replacing the current workaround.

@keszybz
Copy link
Owner Author

keszybz commented Jul 19, 2024

https://bodhi.fedoraproject.org/updates/FEDORA-2024-95ba17d99d for rawhide,
https://bodhi.fedoraproject.org/updates/FEDORA-2024-72fe1a2aa6 for F40.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants