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

Bug 1882852 - remove vendored createprecomplete in iscript and signin… #955

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jcristau
Copy link
Contributor

…gscript

This needed to be kept in sync with the code in mozilla-central, or bad things happen, which bit us in
https://bugzilla.mozilla.org/show_bug.cgi?id=1882322

Instead, directly add remove instructions for the extra signature files we're adding, leaving the rest of the file unchanged.

@jcristau jcristau force-pushed the update-precomplete branch from b92a45a to c1c8558 Compare March 29, 2024 14:39
@jcristau
Copy link
Contributor Author

@jcristau jcristau force-pushed the update-precomplete branch from c1c8558 to 39d5017 Compare April 24, 2024 08:22
@jcristau jcristau requested review from bhearsum and hneiva April 24, 2024 08:23
@jcristau jcristau marked this pull request as ready for review April 24, 2024 08:23
Copy link
Contributor

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

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

I think this is probably OK, but I think there's some risk if a backwards incompatible change is made in the future? Two possible alternatives:

  • Pull createprecomplete from the correct revision, and run that instead.
  • Update/recreate the precomplete file as part of a downstream task (either repackage-signing or a new task). Arguably, we shouldn't be creating the precomplete file until after signing anyways. (This is probably more work than its worth at this time, though.)

@jcristau
Copy link
Contributor Author

I think this is probably OK, but I think there's some risk if a backwards incompatible change is made in the future? Two possible alternatives:

* Pull `createprecomplete` from the correct revision, and run _that_ instead.

This doesn't feel right. The premise of scriptworkers is not to run arbitrary code specified in the payload.

* Update/recreate the precomplete file as part of a downstream task (either `repackage-signing` or a new task). Arguably, we shouldn't be creating the precomplete file until after signing anyways. (This is probably more work than its worth at this time, though.)

Ack, maybe.

@bhearsum
Copy link
Contributor

I think this is probably OK, but I think there's some risk if a backwards incompatible change is made in the future? Two possible alternatives:

* Pull `createprecomplete` from the correct revision, and run _that_ instead.

This doesn't feel right. The premise of scriptworkers is not to run arbitrary code specified in the payload.

Uh...good point! Indeed, this is a bad idea.

* Update/recreate the precomplete file as part of a downstream task (either `repackage-signing` or a new task). Arguably, we shouldn't be creating the precomplete file until after signing anyways. (This is probably more work than its worth at this time, though.)

Ack, maybe.

Just to be clear, I don't think this needs to happen now if it happens at all. Landing this patch is a good improvement at the very least.

@jcristau jcristau force-pushed the update-precomplete branch from 39d5017 to c9a2f66 Compare April 30, 2024 15:28
Copy link
Contributor

@hneiva hneiva left a comment

Choose a reason for hiding this comment

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

I don't know much about precomplete. Syntax LGTM

…gscript

This needed to be kept in sync with the code in mozilla-central, or bad
things happen, which bit us in
https://bugzilla.mozilla.org/show_bug.cgi?id=1882322

Instead, directly add remove instructions for the extra signature files
we're adding, leaving the rest of the file unchanged.
@jcristau jcristau force-pushed the update-precomplete branch from c9a2f66 to ceba27b Compare May 31, 2024 07:06
file = path[1:-1]
if _get_widevine_signing_files([file]):
sigfile = _get_mac_sigpath(file)
fh.write('remove "{}"\n'.format(sigfile))
Copy link
Contributor Author

@jcristau jcristau May 31, 2024

Choose a reason for hiding this comment

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

I'm now worrying that this could add a duplicate remove instruction if the file was already signed in the input archive.

[EDIT: s/not/now/ sigh]

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.

3 participants