build: exclude npm test directories on Windows #23001

richardlau · 2018-09-21T16:38:00Z

npm test directories are excluded on other platforms by
tools/install.py. Do the same on Windows.

Checklist

commit message follows commit guidelines

nodejs-github-bot · 2018-09-21T16:38:01Z

@richardlau build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/959/pipeline

richardlau · 2018-09-21T16:39:34Z

For reference, this is the relevant bit of tools/install.py that excludes the test directories:

node/tools/install.py

Lines 84 to 89 in 058c5b8

    
           # npm has a *lot* of files and it'd be a pain to maintain a fixed list here 
        
           # so we walk its source directory instead... 
        
           for dirname, subdirs, basenames in os.walk('deps/npm', topdown=True): 
        
             subdirs[:] = filter('test'.__ne__, subdirs) # skip test suites 
        
             paths = [os.path.join(dirname, basename) for basename in basenames] 
        
             action(paths, target_path + dirname[9:] + '/')

richardlau · 2018-09-21T16:41:11Z

vcbuild.bat

@@ -332,7 +332,7 @@ copy /Y ..\README.md node-v%FULLVERSION%-win-%target_arch%\ > nul
 if errorlevel 1 echo Cannot copy README.md && goto package_error
 copy /Y ..\CHANGELOG.md node-v%FULLVERSION%-win-%target_arch%\ > nul
 if errorlevel 1 echo Cannot copy CHANGELOG.md && goto package_error
-robocopy /e ..\deps\npm node-v%FULLVERSION%-win-%target_arch%\node_modules\npm > nul
+robocopy ..\deps\npm node-v%FULLVERSION%-win-%target_arch%\node_modules\npm /e /xd test > nul


robocopy documentation has the syntax as robocopy <Source> <Destination> [<File>[ ...]] [<Options>] so I've moved the existing option (/e) to the end of the command line in addition to the new option to exclude test directories.

richardlau · 2018-09-21T16:44:16Z

cc @nodejs/build-files @nodejs/platform-windows @nodejs/npm

richardlau · 2018-09-21T17:37:17Z

Also note this is going to be tricky to test as the regular CI doesn't build the installer/7z/zip packages.

joyeecheung · 2018-09-21T17:40:23Z

There is a https://github.com/nodejs/build/blob/master/jenkins/xml/smoketest-release.xml in the build repo, I wonder what it's for...can we use it? Is it somewhere on Jenkins?

richardlau · 2018-09-21T17:45:34Z

There is a https://github.com/nodejs/build/blob/master/jenkins/xml/smoketest-release.xml in the build repo, I wonder what it's for...can we use it? Is it somewhere on Jenkins?

Not (as far as I can see) on https://ci.nodejs.org/view/All/. Maybe it's on ci-release (which I don't have access to)?

refack · 2018-09-21T18:46:44Z

Any chance we can make install.py windows compatible instead?

nodejs/build:jenkins/xml/smoketest-release.xml@master

Landed under the title jenkins: add job config files for deleted jobs... Which makes sense since the fragile bits in ci-release are WRT to firewalls, certificates, and the specific workers that are mapped to it.

I triggered a test build https://ci-release.nodejs.org/job/iojs+release/3779/

richardlau · 2018-09-21T19:07:02Z

Any chance we can make install.py windows compatible instead?

It's certainly possible but it's not a one line change. We'd have to take into account the differences between the directory layouts on Windows vs the Unix-like platforms and things like symlinks not being used on Windows. Also even though I know it's not a supported platform I wonder if changing install.py for Windows would affect mingw/msys.

#22901 (comment)

richardlau · 2018-09-21T19:08:23Z

I triggered a test build https://ci-release.nodejs.org/job/iojs+release/3779/

Thanks. Could you, or someone else who has access the release ci report back on the results when the build completes?

refack · 2018-09-21T19:57:53Z

No /test/ in npm
node-v11.0.0-rc.0-win-x64.zip

7z l node-v11.0.0-rc.0-win-x64.zip | grep "x64/node_modules/npm/[^/]*$"
2018-05-26 02:00:21 ....A         3258         1471  node-v11.0.0-rc.0-win-x64/node_modules/npm/.mailmap
2018-08-03 02:00:25 ....A          350          195  node-v11.0.0-rc.0-win-x64/node_modules/npm/.npmignore
2018-05-26 02:00:21 ....A         1228          529  node-v11.0.0-rc.0-win-x64/node_modules/npm/.travis.yml
2018-05-24 13:56:30 ....A          993          598  node-v11.0.0-rc.0-win-x64/node_modules/npm/appveyor.yml
2018-09-03 06:00:20 ....A        23475         9843  node-v11.0.0-rc.0-win-x64/node_modules/npm/AUTHORS
2018-09-03 06:00:20 D....            0            0  node-v11.0.0-rc.0-win-x64/node_modules/npm/bin
2018-09-03 06:00:20 ....A        61603        20601  node-v11.0.0-rc.0-win-x64/node_modules/npm/CHANGELOG.md
2018-05-26 02:00:21 D....            0            0  node-v11.0.0-rc.0-win-x64/node_modules/npm/changelogs
2018-05-24 13:56:30 ....A          554          337  node-v11.0.0-rc.0-win-x64/node_modules/npm/configure
2018-09-03 06:00:20 ....A         4566         1715  node-v11.0.0-rc.0-win-x64/node_modules/npm/CONTRIBUTING.md
2018-05-24 13:56:30 D....            0            0  node-v11.0.0-rc.0-win-x64/node_modules/npm/doc
2018-09-03 06:00:20 D....            0            0  node-v11.0.0-rc.0-win-x64/node_modules/npm/html
2018-09-03 06:00:20 D....            0            0  node-v11.0.0-rc.0-win-x64/node_modules/npm/lib
2018-05-24 13:56:30 ....A         9977         3524  node-v11.0.0-rc.0-win-x64/node_modules/npm/LICENSE
2018-05-24 13:56:30 ....A          159          123  node-v11.0.0-rc.0-win-x64/node_modules/npm/make.bat
2018-05-26 02:00:21 ....A         5802         1469  node-v11.0.0-rc.0-win-x64/node_modules/npm/Makefile
2018-05-24 13:56:30 D....            0            0  node-v11.0.0-rc.0-win-x64/node_modules/npm/man
2018-09-03 06:00:21 D....            0            0  node-v11.0.0-rc.0-win-x64/node_modules/npm/node_modules
2018-09-03 06:00:21 ....A         7671         2218  node-v11.0.0-rc.0-win-x64/node_modules/npm/package.json
2018-09-03 06:00:20 ....A         4871         2144  node-v11.0.0-rc.0-win-x64/node_modules/npm/README.md
2018-09-03 06:00:21 D....            0            0  node-v11.0.0-rc.0-win-x64/node_modules/npm/scripts

richardlau · 2018-09-21T21:39:29Z

I realised on the way home that this change only affects the 7z/zip packages. Will need to look at the WiX files to work out if we can do the equivalent for the msi installer.

richardlau · 2018-09-21T21:50:05Z

I think this is the bit that generates the npm files for WiX:

node/tools/msvs/msi/nodemsi.wixproj

Line 76 in 9d71e6a

    
           <HeatDirectory ToolPath="$(WixToolPath)" Directory="..\..\..\deps\npm" PreprocessorVariable="var.NpmSourceDir" DirectoryRefId="NodeModulesFolder" ComponentGroupName="NpmSourceFiles" GenerateGuidsNow="true" SuppressFragments="false" OutputFile="..\..\..\npm.wxs" RunAsSeparateProcess="true">

refack · 2018-09-22T00:12:40Z

I think this is the bit that generates the npm files for WiX:

I think it should work off the directory generated by the :package label, i.e. node-v%FULLVERSION%-win-%target_arch%\node_modules\npm

P.S. I just checked, the msi does indeed contain /npm/test/

richardlau · 2018-09-22T00:54:17Z

Yes, I think working off the directory created for :package will be simpler than the official way to exclude files/directories with heatwhich is to use XSLT (which I fear would scare off even more contributors from the Windows install build files). Would need some refactoring as currently you can skip package and go to msi in which case the directory would not be created.

addaleax · 2018-09-24T20:40:18Z

I realised on the way home that this change only affects the 7z/zip packages. Will need to look at the WiX files to work out if we can do the equivalent for the msi installer.

@richardlau Is this WIP? I’m adding the label but please remove it if I’m mistaken about that.

richardlau · 2018-09-24T20:43:15Z

I realised on the way home that this change only affects the 7z/zip packages. Will need to look at the WiX files to work out if we can do the equivalent for the msi installer.

@richardlau Is this WIP? I’m adding the label but please remove it if I’m mistaken about that.

Yes, thanks. I don't think we should merge this unless the contents of the msi installer match the 7z/zip packages.

joaocgreis · 2018-09-25T01:48:43Z

@richardlau thanks for working on this! The relevant upstream issue is wixtoolset/issues#4023, so there is no easy way to exclude the tests with Heat. It should be easy to get the npm folder from the package folder instead of deps, but vcbuild needs to be adapted to have a common section for package and msi that runs this robocopy line.

richardlau · 2018-09-25T18:43:19Z

Updated:

split part of :package to a :stage_package step that is run if either msi or package is set
replace references to ..\..\..\deps\npm with ..\..\..\Release\node-v$(FullVersion)-win-$(Platform)\node_modules\npm in tools/msvs/msi/nodemsi.wixproj. Note that the existing :package step always staged in Release but there should be no difference wrt npm for debug/release and x64/x86 configurations (i.e. even the debug configurations reference Release to pick up the staged npm) so I've left it always staging in Release.

I gave this a quick build/install locally and no npm and/or npm dependency test directories were installed. If this looks okay, please could someone with access to the release CI do a test build for others to try?

danbev · 2018-09-26T03:51:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17444/

joaocgreis · 2018-09-27T00:53:10Z

Resumed CI: https://ci.nodejs.org/job/node-test-pull-request/17460/
Test build: https://nodejs.org/download/test/v11.0.0-test2018092789c37c2e64/ (https://ci-release.nodejs.org/job/iojs+release/3789/)

joaocgreis · 2018-09-27T01:07:05Z

This should be semver-major. While I can't think of a good reason anything would break, we are removing files from the published packages and installer. This already has the necessary TSC reviews, unless @joyeecheung or @jasnell withdraw their approvals.

joyeecheung

Still LGTM

richardlau · 2018-09-27T22:59:17Z

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/17477/ ✔️

richardlau · 2018-09-28T16:36:48Z

This should be semver-major. While I can't think of a good reason anything would break, we are removing files from the published packages and installer. This already has the necessary TSC reviews, unless @joyeecheung or @jasnell withdraw their approvals.

@jasnell I added a second commit for the msi installer after your approval; does this still look good to you?

The most recent resume CI is green, so I'd like to land this early next week to make the semver-major cutoff for v11.x (#22180 (comment)).

cc @nodejs/tsc (as per https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#involving-the-tsc since this is defensively semver-major).

npm test directories are excluded on other platforms by `tools/install.py`. Do the same on Windows. Fixes: nodejs#22901 PR-URL: nodejs#23001 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com>

richardlau · 2018-10-02T17:22:47Z

Landed in b3b3f53.

nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Sep 21, 2018

richardlau commented Sep 21, 2018

View reviewed changes

richardlau mentioned this pull request Sep 21, 2018

build: windows releases should not include npm tests #22901

Closed

joyeecheung approved these changes Sep 21, 2018

View reviewed changes

jasnell approved these changes Sep 21, 2018

View reviewed changes

refack approved these changes Sep 21, 2018

View reviewed changes

addaleax added the wip Issues and PRs that are still a work in progress. label Sep 24, 2018

richardlau removed the wip Issues and PRs that are still a work in progress. label Sep 25, 2018

richardlau added install Issues and PRs related to the installers. npm Issues and PRs related to the npm client dependency or the npm registry. labels Sep 26, 2018

joaocgreis added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 27, 2018

joaocgreis approved these changes Sep 27, 2018

View reviewed changes

joyeecheung approved these changes Sep 27, 2018

View reviewed changes

richardlau force-pushed the npm-win-no-tests branch from 89c37c2 to d8c67a4 Compare October 2, 2018 17:16

richardlau force-pushed the npm-win-no-tests branch from d8c67a4 to b3b3f53 Compare October 2, 2018 17:20

richardlau merged commit b3b3f53 into nodejs:master Oct 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: exclude npm test directories on Windows #23001

build: exclude npm test directories on Windows #23001

richardlau commented Sep 21, 2018

nodejs-github-bot commented Sep 21, 2018

richardlau commented Sep 21, 2018

richardlau Sep 21, 2018

richardlau commented Sep 21, 2018

richardlau commented Sep 21, 2018

joyeecheung commented Sep 21, 2018

richardlau commented Sep 21, 2018

refack commented Sep 21, 2018

richardlau commented Sep 21, 2018

richardlau commented Sep 21, 2018

refack commented Sep 21, 2018

richardlau commented Sep 21, 2018

richardlau commented Sep 21, 2018

refack commented Sep 22, 2018 •

edited

Loading

richardlau commented Sep 22, 2018

addaleax commented Sep 24, 2018

richardlau commented Sep 24, 2018

joaocgreis commented Sep 25, 2018

richardlau commented Sep 25, 2018 •

edited

Loading

danbev commented Sep 26, 2018

joaocgreis commented Sep 27, 2018

joaocgreis commented Sep 27, 2018

joyeecheung left a comment

richardlau commented Sep 27, 2018 •

edited

Loading

richardlau commented Sep 28, 2018

richardlau commented Oct 2, 2018

build: exclude npm test directories on Windows #23001

build: exclude npm test directories on Windows #23001

Conversation

richardlau commented Sep 21, 2018

Checklist

nodejs-github-bot commented Sep 21, 2018

richardlau commented Sep 21, 2018

richardlau Sep 21, 2018

Choose a reason for hiding this comment

richardlau commented Sep 21, 2018

richardlau commented Sep 21, 2018

joyeecheung commented Sep 21, 2018

richardlau commented Sep 21, 2018

refack commented Sep 21, 2018

richardlau commented Sep 21, 2018

richardlau commented Sep 21, 2018

refack commented Sep 21, 2018

richardlau commented Sep 21, 2018

richardlau commented Sep 21, 2018

refack commented Sep 22, 2018 • edited Loading

richardlau commented Sep 22, 2018

addaleax commented Sep 24, 2018

richardlau commented Sep 24, 2018

joaocgreis commented Sep 25, 2018

richardlau commented Sep 25, 2018 • edited Loading

danbev commented Sep 26, 2018

joaocgreis commented Sep 27, 2018

joaocgreis commented Sep 27, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

richardlau commented Sep 27, 2018 • edited Loading

richardlau commented Sep 28, 2018

richardlau commented Oct 2, 2018

refack commented Sep 22, 2018 •

edited

Loading

richardlau commented Sep 25, 2018 •

edited

Loading

richardlau commented Sep 27, 2018 •

edited

Loading