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

build: strip zip determinism on artifact upload #21756

Merged
merged 1 commit into from
Jan 29, 2020
Merged

build: strip zip determinism on artifact upload #21756

merged 1 commit into from
Jan 29, 2020

Conversation

thypon
Copy link
Contributor

@thypon thypon commented Jan 13, 2020

Description of Change

Since electron zip are including build dates the checksum of each of
these zip files is time dependant. In order to fix this issue strip all
the dates contained in each of the zip entries.

See: #21327 and 97959b5 , cont #20949

Cc: @nornagon @felixrieseberg @ikkisoft

Checklist

Release Notes

Notes: Strip non-determinism from zip release files

@thypon thypon requested a review from a team as a code owner January 13, 2020 11:08
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

Can we instead set the date to something deterministic when we create the zip file? e.g. the date from the commit?

script/release/uploaders/upload.py Show resolved Hide resolved
@thypon
Copy link
Contributor Author

thypon commented Jan 13, 2020

It should be possible to set whichever date we choose, however all the existing tooling tend to assume the minimum epoch time settable in a zip as the default https://salsa.debian.org/reproducible-builds/strip-nondeterminism/blob/master/lib/File/StripNondeterminism/handlers/zip.pm#L200 . If we stick with that convention we will get free external tooling support for checking.

@nornagon
Copy link
Member

@thypon in particular, I meant, can we set the date/time on the zip file when we create it, rather than after the fact? I think we could do so by setting date_time on a ZipInfo object: https://docs.python.org/3/library/zipfile.html#zipinfo-objects

@thypon
Copy link
Contributor Author

thypon commented Jan 13, 2020

I meant, can we set the date/time on the zip file when we create it, rather than after the fact?

Ah that should be the case, yep.

My upload pass covers every case of zip file created, even when we don't directly control the zip creation. Moreover since it is located a step before uploading the release it also opens up for other future metadata removal (eg. OS version header or zero length trailers).

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

Hm, ok, seems reasonable. Zip format probably won't change from underneath us I hope :)

@zcbenz
Copy link
Contributor

zcbenz commented Jan 23, 2020

@thypon Do you mind rebasing this PR on latest master? The macOS CI is failing because of an already fixed issue, rebasing on master should be able to turn it to green.

@thypon
Copy link
Contributor Author

thypon commented Jan 23, 2020

Done, let's wait!

@jkleinsc
Copy link
Contributor

Release builds running here:
AppVeyor release build request for electron-x64 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-x64-release/build/1.0.457
AppVeyor release build request for electron-ia32 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-ia32-release/build/1.0.425
AppVeyor release build request for electron-woa successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-woa-release/build/1.0.249
CircleCI release build request for linux-ia32-publish successful. Check https://circleci.com/gh/electron/electron/444457 for status.
CircleCI release build request for mas-publish successful. Check https://circleci.com/gh/electron/electron/444459 for status.
CircleCI release build request for osx-publish successful. Check https://circleci.com/gh/electron/electron/444458 for status.
CircleCI release build request for linux-arm64-publish successful. Check https://circleci.com/gh/electron/electron/444460 for status.
CircleCI release build request for linux-arm-publish successful. Check https://circleci.com/gh/electron/electron/444461 for status.
CircleCI release build request for linux-x64-publish successful. Check https://circleci.com/gh/electron/electron/444462 for status.

Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Looks like this change is erroring out with:

 File "script\release\uploaders\upload.py", line 277, in <module>
    sys.exit(main())
  File "script\release\uploaders\upload.py", line 73, in main
    upload_electron(release, electron_zip, args)
  File "script\release\uploaders\upload.py", line 213, in upload_electron
    _zero_zip_date_time(file_path)
  File "script\release\uploaders\upload.py", line 175, in _zero_zip_date_time
    archive_size = os.fstat(zip_.fileno()).st_size
AttributeError: 'str' object has no attribute 'fileno'
Command executed with exception:   File "script\release\uploaders\upload.py", line 277, in <module>
    sys.exit(main())
  File "script\release\uploaders\upload.py", line 73, in main
    upload_electron(release, electron_zip, args)
  File "script\release\uploaders\upload.py", line 213, in upload_electron
    _zero_zip_date_time(file_path)
  File "script\release\uploaders\upload.py", line 175, in _zero_zip_date_time
    archive_size = os.fstat(zip_.fileno()).st_size
AttributeError: 'str' object has no attribute 'fileno'

eg see https://ci.appveyor.com/project/electron-bot/electron-ia32-release/builds/30316079

Since electron zip are including build dates the checksum of each of
these zip files is time dependant. In order to fix this issue strip all
the dates contained in each of the zip entries.
@thypon
Copy link
Contributor Author

thypon commented Jan 28, 2020

Seems to be ok now

@jkleinsc
Copy link
Contributor

Release builds running here:
AppVeyor release build request for electron-ia32 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-ia32-release/build/1.0.430
AppVeyor release build request for electron-x64 successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-x64-release/build/1.0.462
AppVeyor release build request for electron-woa successful. Check build status at https://ci.appveyor.com/project/electron-bot/electron-woa-release/build/1.0.254
CircleCI release build request for linux-ia32-publish successful. Check https://circleci.com/gh/electron/electron/448688 for status.
CircleCI release build request for linux-arm64-publish successful. Check https://circleci.com/gh/electron/electron/448691 for status.
CircleCI release build request for linux-arm-publish successful. Check https://circleci.com/gh/electron/electron/448690 for status.
CircleCI release build request for mas-publish successful. Check https://circleci.com/gh/electron/electron/448689 for status.
CircleCI release build request for osx-publish successful. Check https://circleci.com/gh/electron/electron/448692 for status.
CircleCI release build request for linux-x64-publish successful. Check https://circleci.com/gh/electron/electron/448693 for status.

@jkleinsc
Copy link
Contributor

Merging as CI failures unrelated to PR change.

@jkleinsc jkleinsc merged commit 0fe0a08 into electron:master Jan 29, 2020
@release-clerk
Copy link

release-clerk bot commented Jan 29, 2020

Release Notes Persisted

Strip non-determinism from zip release files

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