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 babel #628

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

lookup: add babel #628

wants to merge 7 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Nov 24, 2018

Work in progress because babel is still not compatible with citgm. This is what you have to do now to test it:

make bootstrap
make test

@richardlau
Copy link
Member

Work in progress because babel is still not compatible with citgm. This is what you have to do now to test it:

make bootstrap
make test

Does babel have any equivalent package.json scripts for the make targets? If so we could use #696.

@targos
Copy link
Member Author

targos commented Apr 11, 2019

@targos
Copy link
Member Author

targos commented Apr 11, 2019

Actually no, the bootstrap script is missing from package.json. We could open a PR to add it. Currently this works (and tests all pass with 11.13.0) if yarn is installed:

make bootstrap
npm run test

@richardlau
Copy link
Member

Actually no, the bootstrap script is missing from package.json. We could open a PR to add it.

Maybe that would be simplest and would work with #696 in its current form.

Alternative options include expanding #696 so that instead of being limited to package.json script we allow arbitrary commands (it would make running an alternative package.json script more verbose in the lookup), or we could add yet another option (postinstall?) that could take an arbritrary command to be run after install but before test.

@BridgeAR BridgeAR added the WIP Work in progress label Apr 13, 2019
@MylesBorins MylesBorins changed the base branch from master to main August 14, 2020 21:00
@targos
Copy link
Member Author

targos commented Mar 6, 2021

@codecov-io

This comment has been minimized.

@targos
Copy link
Member Author

targos commented Mar 6, 2021

@targos
Copy link
Member Author

targos commented Mar 6, 2021

It seems like we cannot make it work without babel being a git repository.
/cc @nicolo-ribaudo

https://github.com/targos/citgm/runs/2046109107?check_suite_focus=true

error:                     | ➤ YN0000: │ husky@npm:3.1.0 STDOUT Command failed: git rev-parse --show-toplevel --git-common-dir                                                                       
error:                     | ➤ YN0000: │ husky@npm:3.1.0 STDOUT fatal: not a git repository (or any of the parent directories): .git                                                                 
error:                     | ➤ YN0000: │ husky@npm:3.1.0 STDOUT husky > Failed to install                                                                                                            
error:                     | ::endgroup::                                                                                                                                                            
error:                     | ➤ YN0000: └ Completed in 29s 121ms                                                                                                                                      
error:                     | ➤ YN0000: Done with warnings in 2m 9s                                                                                                                                   
error:                     | rm -f tsconfig.json                                                                                                                                                     
error:                     | git clean packages/*/tsconfig.json -xfq                                                                                                                                 
error:                     | Makefile:113: recipe for target 'clean-tsconfig' failed                                                                                                                 
error:                     |                                                                                                                                                                         
error:                     | fatal: not a git repository (or any of the parent directories): .git                                                                                                    
error:                     | make: *** [clean-tsconfig] Error 128     

@nicolo-ribaudo
Copy link

nicolo-ribaudo commented Mar 6, 2021

I can fix it in our scripts. Do you know if there is any other project in citgm that depends on husky for commit hooks?

lib/lookup.json Outdated
"maintainers": "nicolo-ribaudo",
"prefix": "v",
"yarn": true,
"scripts": ["bootstrap", "build", "test"]

Choose a reason for hiding this comment

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

build shouldn't be needed, it's already done by bootstrap.

@targos
Copy link
Member Author

targos commented Mar 6, 2021

I think the main issue is the git clean command. Husky fails but not in a critical way

@nicolo-ribaudo
Copy link

Actually, it seems yarn install && yarn build && yarn test works because it doesn't cat that git command.

bootstrap is install + clean + build and clean is the step that uses git, but it's not needed with a fresh clone of the repo.

@targos
Copy link
Member Author

targos commented Mar 6, 2021

New run with the install build test sequence: https://github.com/targos/citgm/actions/runs/627579454

@targos
Copy link
Member Author

targos commented Mar 6, 2021

Should we skip on Node.js 10.x ?

@nicolo-ribaudo
Copy link

We test Babel on Node.js 10 so it should work without problems (there are a few disabled tests that rely on native ESM). However, we'll drop Node.js 10 support in a few months.

@targos
Copy link
Member Author

targos commented Mar 6, 2021

Well, it failed on Node.js 10 because of ESM tests. I don't know why they aren't properly disabled but I think we can safely skip Node.js 10. It's in maintenance and the few patches that we will still do to it (if any) are probably not going to break Babel.

@targos
Copy link
Member Author

targos commented Mar 6, 2021

@targos targos changed the title [WIP] lookup: add babel lookup: add babel Mar 7, 2021
@targos
Copy link
Member Author

targos commented Mar 7, 2021

@targos targos removed the WIP Work in progress label Mar 7, 2021
@targos
Copy link
Member Author

targos commented Mar 7, 2021

@nodejs/citgm I think this is ready to land.
There were two errors in the last CI run:
https://ci.nodejs.org/job/citgm-smoker-nobuild/1050/nodes=win10-vs2019/console
https://ci.nodejs.org/job/citgm-smoker-nobuild/1050/nodes=aix71-ppc64/console

But they are not related to babel, as they happen before citgm is even downloaded.

@MylesBorins
Copy link
Member

Hey all, I made a boo boo on main and had to force push. I've rebased this branch and force pushed to make sure that you don't have to do extra work because of my mistake

@targos
Copy link
Member Author

targos commented Jun 21, 2021

New CI: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/149/

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2021

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (460c3a0) 96.46% compared to head (01aedc2) 96.44%.
Report is 32 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #628      +/-   ##
==========================================
- Coverage   96.46%   96.44%   -0.02%     
==========================================
  Files          28       28              
  Lines        2149     2139      -10     
==========================================
- Hits         2073     2063      -10     
  Misses         76       76              

see 6 files with indirect coverage changes

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

lib/lookup.json Outdated Show resolved Hide resolved
@targos
Copy link
Member Author

targos commented May 19, 2024

@nicolo-ribaudo
Copy link

It looks like MacOS CI is failing before even cloning the repo

@targos
Copy link
Member Author

targos commented May 22, 2024

Yes, this is a known issue.
It looks like we just need to skip ppc64le and this is good to go!

@targos
Copy link
Member Author

targos commented May 22, 2024

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/254/console

@targos
Copy link
Member Author

targos commented May 22, 2024

@nodejs/citgm PTAL

@nicolo-ribaudo
Copy link

Finally, awesome, thank you!

@targos
Copy link
Member Author

targos commented Jun 4, 2024

Ping for reviews!

@targos
Copy link
Member Author

targos commented Jun 4, 2024

A babel release came out with the test-only script so I removed "head": true

@targos
Copy link
Member Author

targos commented Jun 4, 2024

https://ci.nodejs.org/job/citgm-smoker-pipeline/255/console

Something weird happened:

08:47:23 info: starting            | @babel/core         
08:47:23 verbose: @babel/core using-uid| 1000                
08:47:23 verbose: @babel/core using-gid| 1000                
08:47:23 verbose: @babel/core using-node| /home/iojs/build/workspace/citgm-smoker-nobuild/node/bin/node
08:47:23 verbose: @babel/core mk.tempdir| /home/iojs/tmp/citgm_tmp/1b8c9956-c19f-4e93-8bd0-f7383ee14d88
08:47:23 verbose: @babel/core npm-view| Retrieving Meta Data
08:47:23 verbose: @babel/core npm-view| Data retrieved      
08:47:23 info: lookup              | @babel/core         
08:47:23 info: lookup-found        | @babel/core         
08:47:23 info: @babel/core lookup-replace| https://github.com/babel/babel/archive/v*.tar.gz
08:47:23 info: @babel/core yarn:   | Downloading project: https://github.com/babel/babel/archive/v*.tar.gz
08:47:24 info: @babel/core npm:    | Project downloaded v*.tar.gz
08:47:24 error: failure             | incorrect header check
08:47:24 info: duration            | test duration: 932ms
08:47:24 error: @babel/core done    | done - the test suite for @babel/core version 7.24.6 failed

@nicolo-ribaudo
Copy link

Maybe the file was corrupted while downloading?

@targos
Copy link
Member Author

targos commented Jun 4, 2024

It's not supposed to download https://github.com/babel/babel/archive/v*.tar.gz

@targos
Copy link
Member Author

targos commented Jun 4, 2024

It's the consequence of two things:

  • The published package doesn't have a gitHead meta information (meaning it was not npm-published from a git repository/commit)
  • detail.fetchSpec is *. I'm looking into why now.

@targos
Copy link
Member Author

targos commented Jun 4, 2024

Here's the relevant part of the code:

citgm/lib/lookup.js

Lines 145 to 160 in 66069f3

const gitHead = rep.head || rep.ignoreGitHead ? null : meta.gitHead;
const sha = context.options.sha || rep.sha || gitHead;
const url = makeUrl(
getRepo(rep.repo, meta),
rep.head ? null : detail.fetchSpec,
meta['dist-tags'],
rep.head ? null : rep.prefix,
sha
);
context.emit(
'data',
'info',
`${context.module.name} lookup-replace`,
url
);
context.module.raw = url;

@targos
Copy link
Member Author

targos commented Jun 4, 2024

Looks like it was broken by https://github.com/npm/npm-package-arg/releases/tag/v10.0.0 and we never realized.

@nicolo-ribaudo
Copy link

We have custom code to generate tarball manifest files, maybe it's a problem on our end?

@targos
Copy link
Member Author

targos commented Jun 4, 2024

Possibly. Has anything changed on that side between 7.24.5 and 7.24.6?

@nicolo-ribaudo
Copy link

No

@targos
Copy link
Member Author

targos commented Jun 4, 2024

Ok, I think it wasn't erroring before because until my last force-push (with "head": true), we were on a three years old branch that didn't include the dependency update!

targos added a commit that referenced this pull request Jun 5, 2024
Convert `fetchSpec` to "latest" when it's "*" to accomodate for
the breaking change that happened in version 10 of the library.
Make lookup tests closer to reality by using the package arg parser
there and constructing fake module metadata that look more like
what the npm API returns.

Refs: #628
Refs: https://github.com/npm/npm-package-arg/releases/tag/v10.0.0
@targos
Copy link
Member Author

targos commented Jun 5, 2024

I opened #1061 to fix that.

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.

None yet

10 participants