Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix build on Windows #1648

Merged
merged 1 commit into from Feb 17, 2020
Merged

Fix build on Windows #1648

merged 1 commit into from Feb 17, 2020

Conversation

raphinesse
Copy link
Contributor

This fixes the package installation and test running on Windows to expose failing tests related to #1643.

More details in the commit messages.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.1%) to 93.717% when pulling ec23455 on raphinesse:fix-windows-build into 45f0860 on benmosher:master.

@coveralls
Copy link

coveralls commented Feb 7, 2020 •
edited

Coverage Status

Coverage remained the same at 97.838% when pulling 8905007 on raphinesse:fix-windows-build into 2beec94 on benmosher:master.

@raphinesse
Copy link
Contributor Author

Apparently using nyc@15 does not support node@<8. Given that those Node.js versions are EOL, this does not seem to be a big issue to me. However, that fact would have to be addressed with further changes.

@ljharb
Copy link
Member

ljharb commented Feb 7, 2020

EOL is irrelevant; we'll support what our supported eslint versions work on.

ljharb
ljharb reviewed Feb 7, 2020
View reviewed changes
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the fix-windows-build branch 2 times, most recently from cd1eae4 to 350eab9 Compare February 7, 2020 19:49
@ljharb @raphinesse
[Tests] appveyor: on node 8-12, use npm 6.10.3
8905007 
Co-authored-by: Raphael von der Grün <raphinesse@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@raphinesse
Copy link
Contributor Author

Playing around with the node or npm version in the CI config might serve to work around the issue of tests not running on Appveyor but it won't resolve the issue for someone cloning the repo to their local Windows machine.

@ljharb
Copy link
Member

ljharb commented Feb 7, 2020

That is true. I'll keep playing with alternatives.

@ljharb
Copy link
Member

ljharb commented Feb 8, 2020

k, looks like adding in-publish, and downgrading npm on node 12, has exposed 50 failing tests, that seem legitimate.

@raphinesse
Copy link
Contributor Author

Regarding your changes to the prepublish script: Is it intended that the build script is neither run on local npm install without any arguments nor on npm pack? You can achieve that by using the built-in prepublishOnly w/out using any additional dependencies. I would consider that less hacky.

@ljharb
Copy link
Member

ljharb commented Feb 8, 2020

prepublishOnly doesn't work on all npm versions, nor does prepare; I'll only ever use prepublish on any project.

@raphinesse
Copy link
Contributor Author

prepublishOnly doesn't work on all npm versions, nor does prepare; I'll only ever use prepublish on any project.

Ah, I see. against the backdrop of trying to support as many npm versions as possible, it makes sense, of course.

What about the other part of my question?

Is it intended that the build script is neither run on local npm install without any arguments nor on npm pack?

@armano2
Copy link

armano2 commented Feb 9, 2020

there seems to be more issues on windows

https://github.com/armano2/test-eslint-monorpo/actions/runs/36409848

@ljharb
Copy link
Member

ljharb commented Feb 9, 2020

@raphinesse no; that's what i added not-in-publish; that way the build script only runs with npm publish, and not with npm install.

@armano2 the current appveyor failures in this PR seem to be legitimate issues on windows, yes.

@ljharb ljharb mentioned this pull request Feb 13, 2020
Merged
@fisker
Copy link
Contributor

fisker commented Feb 14, 2020

I think #1651 is the reason those tests fails

@ljharb
Copy link
Member

ljharb commented Feb 14, 2020

@fisker i rebased #1651 on top of this, and its tests are still failing (albeit with 1 failure instead of 50). Almost there :-)

@raphinesse raphinesse force-pushed the fix-windows-build branch 2 times, most recently from 7e4cd84 to 2df5331 Compare February 14, 2020 08:57
ljharb
ljharb reviewed Feb 14, 2020
View reviewed changes
appveyor.yml Outdated
@@ -22,11 +22,10 @@ install:
# Get the latest stable version of Node.js or io.js
- ps: Install-Product node $env:nodejs_version

# Use npm@6.10.3 to work-around issues w/ nyc@<15 on Windows
- npm install -g npm@6.10.3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please prefer the if-based check; this won't work on node 4, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node 4 is commented out in the Appveyor config. I usually only consider live code. But this branch is exploratory anyhow. I do not find the npm version pinning done here to be a satisfactory solution to the nyc problem (see #1648 (comment)).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but the intention is to uncomment it.

Fair that it's exploratory, but we were down to 1 failing test before you both started force pushing everywhere :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for local dev, the first step is to get CI working - all local dev by maintainers is done on a Mac.

Once CI is working, we can try to fix local Windows dev, for the rare contributor that uses it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but the intention is to uncomment it.

Understood

Fair that it's exploratory, but we were down to 1 failing test before you both started force pushing everywhere :-)

Yes, but tests not running at all on Node 8 and 10.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, first step is to get CI passing somehow.

We can uncomment node 10, 8, 6, 4, etc on appveyor in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for local dev, the first step is to get CI working - all local dev by maintainers is done on a Mac.

Understood

@raphinesse
Copy link
Contributor Author

@ljharb I'm done force-pushing here. Can confirm that npm version pinning resolves the nyc problem on all Node versions currently running on AppVeyor.

ljharb
ljharb approved these changes Feb 14, 2020
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the legwork here! I'll merge this via #1651.

@raphinesse raphinesse closed this Feb 15, 2020
@ljharb ljharb reopened this Feb 15, 2020
@ljharb
Copy link
Member

ljharb commented Feb 15, 2020

It does still have to stay open until it’s merged tho :-)

@ljharb ljharb merged commit 8905007 into import-js:master Feb 17, 2020
@ljharb ljharb mentioned this pull request Feb 17, 2020
Closed
@raphinesse raphinesse deleted the fix-windows-build branch February 18, 2020 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@raphinesse @coveralls @ljharb @armano2 @fisker