WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 9 months ago

Last modified 9 months ago

#38657 closed task (blessed) (fixed)

Shrinkwrap npm dependencies for 4.7

Reported by: netweb Owned by: nacin
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords:
Focuses: Cc:

Description

Follow up once dependancies are finalised in #38199

See also previous milestones: 4.6 in #37541, 4.5 in #36498

Attachments (2)

38657.diff (29.4 KB) - added by nacin 9 months ago.
38657-grunt-shrinkwrap.diff (823 bytes) - added by netweb 9 months ago.

Download all attachments as: .zip

Change History (15)

#1 @netweb
10 months ago

  • Component changed from General to Build/Test Tools
  • Owner set to jorbin
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by jorbin. View the logs.


9 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


9 months ago

This ticket was mentioned in Slack in #core by helen. View the logs.


9 months ago

This ticket was mentioned in Slack in #core by ocean90. View the logs.


9 months ago

#6 @ocean90
9 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 39358:

Build/Test Tools: Add npm-shrinkwrap.json to 4.7.

By shrinkwraping our dependencies, the same versions of everything will be installed no matter what rules the dependency package.json has specified.

Fixes #38657.

#7 @nacin
9 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#8 @nacin
9 months ago

There's a bug in npm-shrinkwrap that prevents the current shrinkwrap from being run on Linux platforms. Specifically, fsevents is needed for something (haven't bothered to figure out what) but it's macOS only. On Linux, nothing is available, and shrinkwrap flips out. There have been some attempts to fix it but it pretty much remains broken upstream.

https://github.com/npm/npm/issues/2679, https://github.com/npm/npm/issues/8707, https://github.com/npm/npm/issues/8921, https://github.com/npm/npm/issues/10768.

Possible workaround: https://github.com/npm/npm/issues/2679#issuecomment-150084700

#9 @nacin
9 months ago

  • Owner changed from jorbin to nacin
  • Status changed from reopened to accepted

Using node 6.9.1 (LTS) and npm 3.10.8, I was able to reproduce a shrinkwrap identical to [39358] on macOS and Linux, with the exception of fsevents being omitted on Linux. (In both scenarios, I also see an irrelevant change to the metadata for the "nan" package.)

When using that Linux-generated shrinkwrap on macOS, node-gyp ends up installing fsevents properly, so it's not needed in npm-shrinkwrap.json.

So, I have a good fix for this, but need to update node and npm on the build server, which I'll do soon.

In the future, we need to be mindful that this thing needs to build on Linux, and probably come up with a way to assert that without it first being discovered when the build server fails. A cheap hack (for this particular problem) that would have identified this quickly would be to validate in unit tests that npm-shrinkwrap.json can't contain the string '"fsevents"'.

@nacin
9 months ago

#10 @netweb
9 months ago

Replying to nacin:

In the future, we need to be mindful that this thing needs to build on Linux, and probably come up with a way to assert that without it first being discovered when the build server fails. A cheap hack (for this particular problem) that would have identified this quickly would be to validate in unit tests that npm-shrinkwrap.json can't contain the string '"fsevents"'.

Currently npm-shrinkwrap.json is never committed to /trunk, it only exists in /branch/x.y, until it is created we can't test it, so on that front @jorbin now that /trunk has branched can you add the 4.7 branch to the Travis CI config please and we can look to adding a test for this.

Also I hit upon grunt-custom-shrinkwrap and added in 38657-grunt-shrinkwrap.diff a new grunt task grunt customShrinkwrap, this creates the npm-shrinkwrap.json file excluding the fsevents dependancy, the npm-shrinkwrap.json file can then be moved manually into /branch/x.y when /trunk is branched as part the new major release workflow.

This patch needs a bit more testing as it reports WordPress 4.8 currently as the task is ran out of /trunk and /trunk has already branched. Also the release workflow handbook docs should document this task if committed

#11 @nacin
9 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 39368:

Build: Remove fsevents from npm-shrinkwrap.json

fsevents is macOS only, and npm-shrinkwrap tries to force it onto other architectures, causing build failures. Upstream: https://github.com/npm/npm/issues/2679.

fixes #38657 for the 4.7 branch.

#12 @nacin
9 months ago

grunt-custom-shrinkwrap is cool but seems overkill for our purposes.

#13 @nacin
9 months ago

In 39480:

Build/Test Tools: Specify exact node version in package.json.

Enforce it in the tests.

Merges [39478] to the 4.7 branch.

see #35105, #38657.

Note: See TracTickets for help on using tickets.