Closed Bug 988114 Opened 7 years ago Closed 7 years ago

Gaia-integration tests pull the tip of the master branch of the gaia-node-modules repository

Categories

(Firefox OS Graveyard :: Gaia, defect, P1)

x86_64
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.4+, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S5 (11apr)
blocking-b2g 1.4+
Tracking Status
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: KWierso, Assigned: hub)

References

Details

(Whiteboard: [c=automation p=2 s= u=])

Attachments

(1 file)

According to https://github.com/mozilla-b2g/gaia/blob/master/Makefile#L711 we're pulling the gaia-node-modules repo's master branch instead of a fixed revision controlled by some in-tree manifest. This recently broke Gi tests on trunk and Aurora trees without warning (previously green runs failed on a retrigger).


https://tbpl.mozilla.org/php/getParsedLog.php?id=36697416&tree=B2g-Inbound

This doesn't match up with the job visibility requirements: https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#8.29_Must_avoid_patterns_known_to_cause_non_deterministic_failures
This arguably justifies re-hiding these tests until resolved, FWIW. Multi-hour bustages across multiple repos with no obvious culprits is not acceptable.
Severity: normal → blocker
What are our options here?
Flags: needinfo?(mhenretty)
Flags: needinfo?(kgrandon)
Flags: needinfo?(gaye)
We can hardcode the sha1 in the branches Makefile.

|make test-perf| should be fine with it.

What do you think?
(In reply to Hubert Figuiere [:hub] from comment #3)
> We can hardcode the sha1 in the branches Makefile.
> 
> |make test-perf| should be fine with it.
> 
> What do you think?

This is an acceptable solution, and something that would be rather quick to implement. Perhaps also a simple gaia_node_modules.revision file that only contained the revision that we want to download?
Flags: needinfo?(kgrandon)
I guess I'm gonna have to wait for this to merge back bug 980541.
Blocks: 980541
I'll take that one. I consider it blocking the one that caused the bustage. Please chime in if you have suggestions.
Assignee: nobody → hub
Status: NEW → ASSIGNED
Whiteboard: [c=automation p= s= u=]
Priority: -- → P1
Thanks for taking this Hub!
NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment] This is to solve a potential bustage when we update the modules. We MUST uplift this to be useful. Today changes wrecked havoc and we had to revert them as the tree was closed.
[Bug caused by] (feature/regressing bug #): Reverted 980541 and etc...
[User impact] if declined: Tree breakage, unable to fix 980541, etc
[Testing completed]: manually tested the various combinations of the make file.
[Risk to taking this patch] (and alternatives if risky): low: build system
[String changes made]: NONE
Attachment #8396911 - Flags: review?(kgrandon)
Attachment #8396911 - Flags: approval-gaia-v1.4?
Attachment #8396911 - Flags: approval-gaia-v1.3?
Comment on attachment 8396911 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17624

Yuren, would you mind to review this? Thanks. Kevin said you'd be a better reviewer.

Thanks !
Attachment #8396911 - Flags: review?(kgrandon) → review?(yurenju.mozilla)
Travis' native caching capability uses the branch names. I wonder if it's not possible to do the same here? Although hardcoding the hash would work for a start.
Comment on attachment 8396911 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17624

cancelling review. Travis is red.
Attachment #8396911 - Flags: review?(yurenju.mozilla)
gaia_node_modules.revision containing the hash we need works for me. This also allows us to fetch different node_module revision per gaia branch/train, so it's something we've needed anyway.
Flags: needinfo?(mhenretty)
Comment on attachment 8396911 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17624

Yuren,

I have updated the pull request since last request. It is green on Travis. Do you mind reviewing this?

Thanks
Attachment #8396911 - Flags: review?(yurenju.mozilla)
Whiteboard: [c=automation p= s= u=] → [c=automation p=2 s= u=]
Just thought of a concern here: what happens when we switch branches? 1.3 to 1.4 to master to 1.3 to master to 1.4 to master?

Think also of the offline situation...
The idea is to change gaia_node_modules.revision for each branch - which is why I requested approval.

Indeed if you are offline and switch branches (or the revision change) it might not work out well :-/ But then |./repo sync| doesn't work offline either.

(the whole thing use to fail without internet connection anyway)
we don't need to do ./repo sync when we're offline. However, switching branches is very common. I don't want to download the tar again each time I cherry-pick.

If we didn't care about windows, I'd ask to use a directory to keep the various node_modules, and just link. Even this could fail if we do "npm install".

The bottom line is: this is very fragile, and I don't have a solution ;)
(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #14)
> Just thought of a concern here: what happens when we switch branches? 1.3 to
> 1.4 to master to 1.3 to master to 1.4 to master?
> 
> Think also of the offline situation...

We had this same problem before we moved node_modules into its own repo. Each branch had it's own package.json, and so switching branches and running npm install would fail if offline. The only reason why the offline case "worked" after moving node_modules into its own repo is that all branches pulled the same node_modules, which is also wrong.

I agree, it would be wonderful if we could cache node_modules for our different branches (node_modules_mater, node_modules_1_3, etc), and then perhaps symlink/rebuild when running make. Or something like that. But in my opinion, that shouldn't block this bug.
(In reply to Michael Henretty [:mhenretty] from comment #17)
> (In reply to Julien Wajsberg [:julienw] (away until March 24) from comment
> #14)
> > Just thought of a concern here: what happens when we switch branches? 1.3 to
> > 1.4 to master to 1.3 to master to 1.4 to master?
> > 
> > Think also of the offline situation...
> 
> We had this same problem before we moved node_modules into its own repo.
> Each branch had it's own package.json, and so switching branches and running
> npm install would fail if offline. The only reason why the offline case
> "worked" after moving node_modules into its own repo is that all branches
> pulled the same node_modules, which is also wrong.

MMm right, I think I commented "npm install" in these cases, and maybe I could do the same with the new situation.
I had done a patch I never submitted to set an OFFLINE env to tell us to not bother with npm. I believe this is worth filing.
Yuren,

I'll be in Taipei by Monday. But we really need to get that in. If you feel you can't review that, let me know, I'm sure we can find another reviewer. Kevin Grandon said you were best for that.
Flags: needinfo?(yurenju.mozilla)
(In reply to Hubert Figuiere [:hub] from comment #8)
> Created attachment 8396911 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17624
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment] This is to solve a potential bustage when we
> update the modules. We MUST uplift this to be useful. Today changes wrecked
> havoc and we had to revert them as the tree was closed.
> [Bug caused by] (feature/regressing bug #): Reverted 980541 and etc...
> [User impact] if declined: Tree breakage, unable to fix 980541, etc
> [Testing completed]: manually tested the various combinations of the make
> file.
> [Risk to taking this patch] (and alternatives if risky): low: build system
> [String changes made]: NONE

Typically, only blocking-bug:1.3+ can land on the 1.3 branch. So please nominate the blocking-beg: 1.3? with justification of why its a blocker so triage can make a call here.

Also to land it on master this first needs to get review/landed on master. So waiting for that to happen before I consider it on 1.4 uplift and have the builds. Is this test/automation/build only change and no risk to users ? Once this is on master we should verify builds spawn as expected.
Flags: needinfo?(hub)
blocking-b2g: --- → 1.3?
Flags: needinfo?(hub)
It is blocking 1.3, 1.3T and 1.4 because this change is needed for the test infrastructure to make sure we freeze the dependencies. This does not impact the shipping code, but this impact
[sorry I got cut here]
... but this impact testing. We have had a tree closure caused by a change in these modules (not in gaia) that broke also testing. The change itself is in gaia.

We need 1.3, 1.3T and 1.4.
Actually need this in 1.4. 1.3 doesn't use gaia-node-modules. My bad. (it will make things easier)
blocking-b2g: 1.3? → 1.4?
Attachment #8396911 - Flags: approval-gaia-v1.3?
(In reply to bhavana bajaj [:bajaj] from comment #21)
> (In reply to Hubert Figuiere [:hub] from comment #8)
> > Created attachment 8396911 [details] [review]
> > Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17624
> > 
> > NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> > better understand the B2G approval process and landings.
> > 
> > [Approval Request Comment] This is to solve a potential bustage when we
> > update the modules. We MUST uplift this to be useful. Today changes wrecked
> > havoc and we had to revert them as the tree was closed.
> > [Bug caused by] (feature/regressing bug #): Reverted 980541 and etc...
> > [User impact] if declined: Tree breakage, unable to fix 980541, etc
> > [Testing completed]: manually tested the various combinations of the make
> > file.
> > [Risk to taking this patch] (and alternatives if risky): low: build system
> > [String changes made]: NONE
> 
> Typically, only blocking-bug:1.3+ can land on the 1.3 branch. So please
> nominate the blocking-beg: 1.3? with justification of why its a blocker so
> triage can make a call here.
> 
> Also to land it on master this first needs to get review/landed on master.
err, meant 1.4 above. %/s/land to master/land to 1.4
> So waiting for that to happen before I consider it on 1.4 uplift and have
> the builds. Is this test/automation/build only change and no risk to users ?
> Once this is on master we should verify builds spawn as expected.
It is for 1.4 and master only. In mistakenly thought I need it for 1.3, but I don't.

I'm waiting on review still, but anticipating approval.
Hub, Sorry I'm pretty busy last week, I'm revewing your patch now.
Flags: needinfo?(yurenju.mozilla)
Hi Hubert, the pull request looks good but I have a question for modules-fetch-stamp, we may discuss it today.
Flags: needinfo?(hub)
Comment on attachment 8396911 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17624

looks good to me for updated pull request if nit is addressed, don't forget rebase to one commit before land it :-)
Attachment #8396911 - Flags: review?(yurenju.mozilla) → review+
Flags: needinfo?(hub)
Thanks !

Will rebase / squash. Address the nit. Will ensure Travis and green and TBPL.

(aka it'll have to wait a bit later today)
Flags: needinfo?(gaye)
Merged

https://github.com/mozilla-b2g/gaia/commit/7e8fa687f5a383740619517dba17174669a34164
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
This has landed to gaia master. To be useful it needs to land on 1.4.

Request both blocking and approval. Waiting on both.
Since the node modules are used only in a dev environment, and not at all for building, I'd say we don't necessarily need a blocking or an approval to land. a=npotb (not part of the build) should be enough.
+1. This is basically a requirement for dev to function moving forward, so there is no way we can't have this for 1.4. a=nbotb+julienw+kgrandon should be plenty here.
1.4+ to clear the way...
blocking-b2g: 1.4? → 1.4+
Attachment #8396911 - Flags: approval-gaia-v1.4? → approval-gaia-v1.4+
You need to log in before you can comment on or make changes to this bug.