975233 - use |tar --strip 1| instead of |tar && mv && rm| in Makefile

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Kevin saved gaia from hell with downloading npm tarball from github, now it's time to polish this command.

we should use |tar --strip 1| instead of |tar && mv && rm| in Makefile[1]

[1] https://github.com/mozilla-b2g/gaia/blob/e542540cfa64c31ac8bb71b1a9cf16879fc9d58e/Makefile#L681-L693

Comment 1

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

set needinfo flag to me on bottom of coment box if you are interested in, check the checkbox which is left of "need more information from other" and fill my email.

Comment 2

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Reorganize the rules for installing node.js modules

This patch uses --strip-components to extract only the node_modules directory and do so in the right spot but also reorganizes the rule around it to fix a few common issues:

- if the node_modules directory is not present but modules.tar is then we didn't rebuild node_modules, this is obviously wrong

- we extracted node_modules without touching it so it appeared as always old from make POV, this is also addressed by passing the |-m| flag to the tar invocation

- the node_modules rule depended on $(NPM_INSTALLED_PROGRAMS) and not vice-versa, so if you tried to explicitly |make node_modules| you'd get three invocations of the $(NPM_INSTALLED_PROGRAMS) rule which is wrong and lead to funny errors (like trying to download three copies of the tarball at the same time)

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Reorganize the rules for installing node.js modules

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Reorganize the rules for installing node.js modules

Scratch the previous patch, apparently most recent versions of GNU tar explicitly require you to pass the |--wildcards| option if you intend to use wildcards so here goes an updated patch that deals with them.

Comment 5

[Gabriele Svelto [:gsvelto]]

Green Travis run is here:

https://travis-ci.org/mozilla-b2g/gaia/builds/19340178

Comment 6

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8379737 [details] [diff] [review]
[PATCH v2] Reorganize the rules for installing node.js modules

Review of attachment 8379737 [details] [diff] [review]:
-----------------------------------------------------------------

r=yurenju if nit is addressed, thank you, Gabriele!

::: Makefile
@@ +689,5 @@
> +
> +node_modules: modules.tar
> +	$(TAR_WILDCARDS) --strip-components 1 -x -m -f modules.tar "mozilla-b2g-gaia-node-modules-*/node_modules"
> +	npm install && npm rebuild
> +	echo "node_modules installed."

add "@" before |echo| because we don't need to print command itself.

Comment 7

[Gabriele Svelto [:gsvelto]]

Thanks for the review! I've turned the |echo| command into |@echo| and I'm giving this a spin on TBPL to make sure I'm not breaking anything as our automation scripts now rely on the |node_modules| target (see bug 976251):

https://tbpl.mozilla.org/?tree=Try&rev=c170663117da

Comment 8

[Gabriele Svelto [:gsvelto]]

The previous TBPL run had some broken parameters for the custom gaia repo, I've fixed it and the new run is here:

https://tbpl.mozilla.org/?tree=Try&rev=8b40ddd626f0

In the meantime this is the green Travis run:

https://travis-ci.org/mozilla-b2g/gaia/builds/19560002

Comment 9

[Gabriele Svelto [:gsvelto]]

The TBPL test is looking good too, pushed to gaia/master 80d6405725788327102cab36e8d8c017cf25fb23

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cf337c5ee1ef

Comment 11

[Hubert Figuiere [:hub]]

v1.3t uplift (gaia only)
https://github.com/mozilla-b2g/gaia/commit/3babb9182ebaab396d3727ed163e38e077e43156