Closed Bug 1441495 Opened 2 years ago Closed 2 years ago

Remove signing support from mozharness

Categories

(Release Engineering :: Applications: MozharnessCore, enhancement)

enhancement
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
Tracking Status
firefox61 --- fixed

People

(Reporter: catlee, Assigned: vprabhu, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=py])

Attachments

(1 file)

mozharness includes support for setting up the build environment to communicate with the signing infrastructure. e.g.

https://searchfox.org/mozilla-central/search?q=signing_server&path=testing/mozharness

Since we're not doing any of our builds in buildbot any more, this is no longer needed. We should remove the code in mozharness that handles MOZ_SIGNING_SERVERS and MOZ_SIGN_CMD.
can i work on this issue ??
(In reply to simtmbd from comment #1)
> can i work on this issue ??

Of course! Have you worked with Firefox source code or mozharness before?
(In reply to simtmbd from comment #1)
> can i work on this issue ??

Let me know if you are not working on this. I have already worked on my first bug in mozharness. I will be able to take it up.
(In reply to Chris AtLee [:catlee] from comment #2)
> (In reply to simtmbd from comment #1)
> > can i work on this issue ??
> 
> Of course! Have you worked with Firefox source code or mozharness before?

Hi Chris,

Do I have to remove the other helper methods too or just the ones where MOZ_SIGNING_SERVERS and MOZ_SIGN_CMD is used? 

Also, in https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/signing.py,

is the query_moz_sign_cmd method needed? https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/testing/mozharness/mozharness/mozilla/signing.py#40
(In reply to Venkatesh Prabhu :vprabhu from comment #4)
> (In reply to Chris AtLee [:catlee] from comment #2)
> > (In reply to simtmbd from comment #1)
> > > can i work on this issue ??
> > 
> > Of course! Have you worked with Firefox source code or mozharness before?
> 
> Hi Chris,
> 
> Do I have to remove the other helper methods too or just the ones where
> MOZ_SIGNING_SERVERS and MOZ_SIGN_CMD is used? 
> 
> Also, in
> https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/
> mozilla/signing.py,
> 
> is the query_moz_sign_cmd method needed?
> https://searchfox.org/mozilla-central/rev/
> 61d400da1c692453c2dc2c1cf37b616ce13dea5b/testing/mozharness/mozharness/
> mozilla/signing.py#40

That entire file can go away. Both of the mixins and the helper methods they provide aren't used any more.
(In reply to Chris AtLee [:catlee] from comment #5)
> (In reply to Venkatesh Prabhu :vprabhu from comment #4)
> > (In reply to Chris AtLee [:catlee] from comment #2)
> > > (In reply to simtmbd from comment #1)
> > > > can i work on this issue ??
> > > 
> > > Of course! Have you worked with Firefox source code or mozharness before?
> > 
> > Hi Chris,
> > 
> > Do I have to remove the other helper methods too or just the ones where
> > MOZ_SIGNING_SERVERS and MOZ_SIGN_CMD is used? 
> > 
> > Also, in
> > https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/
> > mozilla/signing.py,
> > 
> > is the query_moz_sign_cmd method needed?
> > https://searchfox.org/mozilla-central/rev/
> > 61d400da1c692453c2dc2c1cf37b616ce13dea5b/testing/mozharness/mozharness/
> > mozilla/signing.py#40
> 
> That entire file can go away. Both of the mixins and the helper methods they
> provide aren't used any more.

Okay. Everything else looks okay to me except the query_l10n_env method:

https://searchfox.org/mozilla-central/rev/61d400da1c692453c2dc2c1cf37b616ce13dea5b/testing/mozharness/scripts/desktop_l10n.py#412

It says 'bootstrap_env is set with query_moz_sign_cmd()' which is defined in signing.py(which goes away as you have mentioned.) Should I remove the call to query_bootstrap_env()? (l10n_env.update(self.query_bootstrap_env())).

If I do remove it, will query_l10n_env() get affected? This again has been called a few times in the same file.
I think you can just remove that comment...I believe the code should work still.
(In reply to Chris AtLee [:catlee] from comment #7)
> I think you can just remove that comment...I believe the code should work
> still.

Thanks Chris. Let me know if I should run any tests. I wasn't sure, so directly submitted the changes for review.
(In reply to Venkatesh Prabhu :vprabhu from comment #9)
> (In reply to Chris AtLee [:catlee] from comment #7)
> > I think you can just remove that comment...I believe the code should work
> > still.
> 
> Thanks Chris. Let me know if I should run any tests. I wasn't sure, so
> directly submitted the changes for review.

Yeah, if you could push your changes to Try, that would be great!
(In reply to Chris AtLee [:catlee] from comment #10)
> (In reply to Venkatesh Prabhu :vprabhu from comment #9)
> > (In reply to Chris AtLee [:catlee] from comment #7)
> > > I think you can just remove that comment...I believe the code should work
> > > still.
> > 
> > Thanks Chris. Let me know if I should run any tests. I wasn't sure, so
> > directly submitted the changes for review.
> 
> Yeah, if you could push your changes to Try, that would be great!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3168c1b47544714cd286a47fd11519b6e5b81ac8
Looks like you need to clean up at least one import:

[task 2018-03-06T04:43:58.926Z] Traceback (most recent call last):
[task 2018-03-06T04:43:58.926Z]   File "/builds/worker/workspace/build/src/testing/mozharness/scripts/fx_desktop_build.py", line 25, in <module>
[task 2018-03-06T04:43:58.926Z]     from mozharness.mozilla.building.buildbase import BUILD_BASE_CONFIG_OPTIONS, \
[task 2018-03-06T04:43:58.926Z]   File "/builds/worker/workspace/build/src/testing/mozharness/mozharness/mozilla/building/buildbase.py", line 44, in <module>
[task 2018-03-06T04:43:58.926Z]     from mozharness.mozilla.signing import SigningMixin
[task 2018-03-06T04:43:58.926Z] ImportError: No module named signing
(In reply to Chris AtLee [:catlee] from comment #12)
> Looks like you need to clean up at least one import:
> 
> [task 2018-03-06T04:43:58.926Z] Traceback (most recent call last):
> [task 2018-03-06T04:43:58.926Z]   File
> "/builds/worker/workspace/build/src/testing/mozharness/scripts/
> fx_desktop_build.py", line 25, in <module>
> [task 2018-03-06T04:43:58.926Z]     from
> mozharness.mozilla.building.buildbase import BUILD_BASE_CONFIG_OPTIONS, \
> [task 2018-03-06T04:43:58.926Z]   File
> "/builds/worker/workspace/build/src/testing/mozharness/mozharness/mozilla/
> building/buildbase.py", line 44, in <module>
> [task 2018-03-06T04:43:58.926Z]     from mozharness.mozilla.signing import
> SigningMixin
> [task 2018-03-06T04:43:58.926Z] ImportError: No module named signing

I fixed import error in few files but one of the class params is invoking a method from signing.py in these files.

https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/building/buildbase.py#671

Here's the try results for linux platform only. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b22ca8998e8fe2eeae05ca8c26b16ca7798514c
(In reply to Venkatesh Prabhu :vprabhu from comment #13)
> I fixed import error in few files but one of the class params is invoking a
> method from signing.py in these files.
> 
> https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/
> mozilla/building/buildbase.py#671
> 
> Here's the try results for linux platform only.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6b22ca8998e8fe2eeae05ca8c26b16ca7798514c

Ok, great! I think we just need to remove SigningMixin everywhere now.
(In reply to Chris AtLee [:catlee] from comment #14)
> (In reply to Venkatesh Prabhu :vprabhu from comment #13)
> > I fixed import error in few files but one of the class params is invoking a
> > method from signing.py in these files.
> > 
> > https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/
> > mozilla/building/buildbase.py#671
> > 
> > Here's the try results for linux platform only.
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=6b22ca8998e8fe2eeae05ca8c26b16ca7798514c
> 
> Ok, great! I think we just need to remove SigningMixin everywhere now.

Done. Had to remove MobileSigningMixin too in one file. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc99ef74404812e9eb29f3c511aad64deeacebbb
Blocks: 1443938
Assignee: nobody → venkateshprabhu2
(In reply to Chris AtLee [:catlee] from comment #14)
> (In reply to Venkatesh Prabhu :vprabhu from comment #13)
> > I fixed import error in few files but one of the class params is invoking a
> > method from signing.py in these files.
> > 
> > https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/
> > mozilla/building/buildbase.py#671
> > 
> > Here's the try results for linux platform only.
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=6b22ca8998e8fe2eeae05ca8c26b16ca7798514c
> 
> Ok, great! I think we just need to remove SigningMixin everywhere now.

I have added the two query methods in script.py. 
I imported scriptmixin thinking it might be needed, but wasn't sure. 

Here are the try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1d3d55c4ce2bd4208037ee394841cf6f4905414
(In reply to Chris AtLee [:catlee] from comment #14)
> (In reply to Venkatesh Prabhu :vprabhu from comment #13)
> > I fixed import error in few files but one of the class params is invoking a
> > method from signing.py in these files.
> > 
> > https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/
> > mozilla/building/buildbase.py#671
> > 
> > Here's the try results for linux platform only.
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=6b22ca8998e8fe2eeae05ca8c26b16ca7798514c
> 
> Ok, great! I think we just need to remove SigningMixin everywhere now.

I have cleaned it up and removed the import statements. Here are the linux build results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf8e3ccf809074270899cb1bdaa5d700b9f01408

Let me know if I should run all the mochitests.
Looks good!

I think https://hg.mozilla.org/try/rev/25c64c147662a43f1515127619c037e97d63b15b#l2.12 can still be removed.

And yeah, let's do a full try run to make sure we haven't missed any edge cases. `mach try -b do -p all -u all` should do it.
(In reply to Chris AtLee [:catlee] from comment #18)
> Looks good!
> 
> I think
> https://hg.mozilla.org/try/rev/25c64c147662a43f1515127619c037e97d63b15b#l2.
> 12 can still be removed.
> 
> And yeah, let's do a full try run to make sure we haven't missed any edge
> cases. `mach try -b do -p all -u all` should do it.

Here are the results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bdccc1147f74946df779918b8903e6c9243423c
Looks good to me!

Can you post the latest version of the patch to mozreview, and then we can get it reviewed and landed!
Comment on attachment 8955741 [details]
Bug 1441495 - Removed signing support from mozharness.

https://reviewboard.mozilla.org/r/224820/#review233274
Attachment #8955741 - Flags: review?(catlee) → review+
Pushed by catlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/86d8aed616d4
Removed signing support from mozharness. r=catlee
https://hg.mozilla.org/mozilla-central/rev/86d8aed616d4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
(In reply to Chris AtLee [:catlee] from comment #20)
> Looks good to me!
> 
> Can you post the latest version of the patch to mozreview, and then we can
> get it reviewed and landed!

Can you point to me, similar bugs if and when you come across these so that I can work on them? I will continue looking for unassigned bugs actively.
You need to log in before you can comment on or make changes to this bug.