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.

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

lookup: add next #1044

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

lookup: add next #1044

wants to merge 3 commits into from

Conversation

styfle
Copy link
Member

@styfle styfle commented Jan 9, 2024

Checklist
  • npm test passes
  • contribution guidelines followed
    here
Related

Co-authored-by: Ethan Arrowood [email protected]

@styfle styfle changed the title add next lookup: add next Jan 9, 2024
@styfle styfle marked this pull request as ready for review January 9, 2024 19:42
@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (419202a) 96.33% compared to head (a693d85) 95.04%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1044      +/-   ##
==========================================
- Coverage   96.33%   95.04%   -1.29%     
==========================================
  Files          28       28              
  Lines        2181     2181              
==========================================
- Hits         2101     2073      -28     
- Misses         80      108      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Looks good assuming you can get CI to pass. That might be something that needs fixing on the Next side, though, if some of the tests are flaky; maybe a different test command can be created that’s essentially “run the tests that we think should always pass.”

@styfle
Copy link
Member Author

styfle commented Jan 9, 2024

@GeoffreyBooth It looks like windows has been failing on main for awhile https://github.com/nodejs/citgm/commits/main

Furthermore, I'm not sure if citgm actually runs any tests against the lookup.json file that I modified.

@Ethan-Arrowood
Copy link

@GeoffreyBooth does the CI run citgm-all ? I think that's the only way lookup.json is used, right?

@GeoffreyBooth
Copy link
Member

I’m not very knowledgeable of how CITGM works. Perhaps @targos knows?

@targos
Copy link
Member

targos commented Jan 9, 2024

Here's a first test run: https://github.com/nodejs/citgm/actions/runs/7466807218

@targos
Copy link
Member

targos commented Jan 9, 2024

CI doesn't run citgm-all. It takes too long and must be run manually.

@Ethan-Arrowood
Copy link

Looks like it didn't install correctly: https://github.com/nodejs/citgm/actions/runs/7466807218/job/20319001003#step:5:12

@styfle
Copy link
Member Author

styfle commented Jan 9, 2024

This should fix it: vercel/next.js#60443

styfle added a commit to vercel/next.js that referenced this pull request Jan 9, 2024
The postinstall script was failing In the case when you download [a
zip](https://codeload.github.com/vercel/next.js/zip/refs/heads/canary)
of the repo.

This PR fixes it so that `git config` can fail silently in that case
when the repo is not using git.

- Related to nodejs/citgm#1044

Closes NEXT-2036
@styfle
Copy link
Member Author

styfle commented Jan 9, 2024

@targos Can you try once more now that its fixed?

@targos
Copy link
Member

targos commented Jan 10, 2024

@Ethan-Arrowood
Copy link

Looks like we may need to skip on windows.

Otherwise it seems to have passed on mac and linux 🎉

@styfle
Copy link
Member Author

styfle commented Jan 10, 2024

@targos I added skip win32. Please run once more (Third time's the charm ☘️ )

@targos
Copy link
Member

targos commented Jan 10, 2024

OK, let's do a full run in Jenkins.

@styfle
Copy link
Member Author

styfle commented Jan 10, 2024

@targos Looks like pnpm support isn't quite working. Its not selecting the correct version of pnpm

https://ci.nodejs.org/job/citgm-smoker-nobuild/nodes=rhel8-x64/1580/testReport/(root)/citgm/next_v14_0_4/

ERR_PNPM_UNSUPPORTED_ENGINE Unsupported environment (bad pnpm and/or Node.js version)
 Your pnpm version is incompatible with "/home/iojs/tmp/citgm_tmp/12755dbb-db09-4e8a-975e-85e8c56c01ad/next".
 Expected version: 8.14.0
 Got: 8.14.1

I think we need to make sure corepack enable pnpm is run as the first step so the correct version is selected from package.json https://nodejs.org/api/corepack.html

@targos
Copy link
Member

targos commented Jan 11, 2024

pnpm is not installed on CI hosts. It's a dependency of the citgm package: 46d35ae

@styfle
Copy link
Member Author

styfle commented Jan 11, 2024

@targos Interesting. Then perhaps citgm should utilize corepack. Or at least manually read the packageManger field in package.json to select the correct version of the package manger.

@GeoffreyBooth
Copy link
Member

@targos Interesting. Then perhaps citgm should utilize corepack. Or at least manually read the packageManger field in package.json to select the correct version of the package manger.

Why does it need to use a precise version of pnpm? Surely 8.14.0 and 8.14.1 aren’t that different?

@styfle
Copy link
Member Author

styfle commented Jan 11, 2024

@GeoffreyBooth Next.js is a highly visible project that receives many PRs. We lock down the package manager version so that contributors fail fast when they have the wrong pnpm installed rather than then submitting an issue saying it doesn't work and we have to ask for system details, etc.

In theory, a patch version should't matter, but in practice it can and there is no reason to leave it open for interpretation. Thats probably the same reason why the packageManager field doesn't allow ranges - it has to be exact.

Thankfully, there is a tool to solve this now: corepack. So most of the citgm logic for package manager selection could probably be deferred to corepack.

@GeoffreyBooth
Copy link
Member

Okay, do you want to update CITGM accordingly?

@Ethan-Arrowood
Copy link

Would that be this workflow file?

run: npm install
Where is the one that Node.js uses?

@styfle
Copy link
Member Author

styfle commented Jan 11, 2024

I can’t seem to find the place where package managers are installed? Can you share the line of code?

@GeoffreyBooth
Copy link
Member

They're just dependencies of CITGM, so they'd get installed via that.

@styfle
Copy link
Member Author

styfle commented Mar 9, 2024

Should I add corepack support to citgm?

Perhaps right before install here?

const proc = spawn(packageManagerBin, args, options);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support pnpm; add Next.js
5 participants