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

chore: fix CI pipeline issues with deduped tar package #30077

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Aug 21, 2024

Additional details

After introducing our yarn dedupe strategy to highest after our @nrwl/nx-cloud upgrade, the yarn.lock file became out of sync.

deduping the tar package for semver packages to point to 6.2.1 (introduced in the @nrwl/nx-cloud update) instead of 6.1.15 causes the issue:

TypeError: Class extends value #<Object> is not a constructor or null

inside our ci pipeline. This issue is hard to reproduce locally as well as in the used docker container. I was also unable to find a root cause of this issue.

To remedy this, I am introducing a yarn resolution to resolve tar in the nx-cloud package from 6.2.1 to 6.1.15, which removes the reference to 6.2.1 and gets our CI pipeline working again. However, this may creep up again in the future if a semver compatible package of tar gets updated to 6.2.1 and other tar versions are deduped.

I would rather solve this issue by telling the nx-cloud package to not hoist any dependencies and install them all inside the nx-cloud package to prevent cross pollination, but I have not been able to figure that out.

Steps to test

How has the user experience changed?

PR Tasks

@AtofStryker AtofStryker changed the title does this fix the tar issues? [run ci] fix: CI pipeline issues with deduped tar package Aug 21, 2024
@AtofStryker AtofStryker changed the title fix: CI pipeline issues with deduped tar package chore: fix CI pipeline issues with deduped tar package Aug 21, 2024
@AtofStryker AtofStryker self-assigned this Aug 21, 2024
@AtofStryker
Copy link
Contributor Author

@MikeMcC399 this should resolve the CI failures we are seeing and updates the lock. Fix is not ideal but it does get us going.

Copy link

cypress bot commented Aug 21, 2024

cypress    Run #56699

Run Properties:  status check passed Passed #56699  •  git commit 755939a384: chore: fix CI pipeline issues with deduped tar package
Project cypress
Branch Review fix/yarn_lock
Run status status check passed Passed #56699
Run duration 18m 10s
Commit git commit 755939a384: chore: fix CI pipeline issues with deduped tar package
Committer AtofStryker
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 1024
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 10246
View all changes introduced in this branch ↗︎
UI Coverage  null%
  Untested elements 0  
  Tested elements 0  
Accessibility  null%
  Failed rules  0 critical   0 serious   0 moderate   0 minor
  Failed elements 0  

@MikeMcC399
Copy link
Contributor

@AtofStryker

I guess this fixes nxcloud caching. Good you found where it was going wrong and you now have a workaround!

To remedy this, I am introducing a yarn resolution to resolve tar in the nx-cloud package from 6.2.1 to 6.15.1, which removes the reference to 6.2.1 and gets our CI pipeline working again.

There's a typo in that sentence it should be 6.1.15 ([email protected]). It's downgrading the tar version to the one used previously by @nrwl/[email protected].

@MikeMcC399
Copy link
Contributor

@AtofStryker

I haven't actually understood why nx-cloud is getting in a mess. I did notice though that postinstall deduplication is only used for local runs, not for CI, so I'm wondering perhaps if this is making nx-cloud run in an inconsistent state? The logs that I can see don't show the level of detail to be able to work this out however.

const postInstallCommands = {
local: 'patch-package && yarn-deduplicate --strategy=highest && lerna run rebuild-better-sqlite3 --scope @packages/server && yarn build && yarn build-v8-snapshot-dev',
ci: 'patch-package && lerna run rebuild-better-sqlite3 --scope @packages/server',
}

@jennifer-shehane
Copy link
Member

@MikeMcC399 Yah we were commenting on the same thing yesterday about ci vs local. We kind of need to move to a better package manager because we keep running into weird conflicts.

@MikeMcC399
Copy link
Contributor

@jennifer-shehane

Yah we were commenting on the same thing yesterday about ci vs local. We kind of need to move to a better package manager because we keep running into weird conflicts.

@AtofStryker
Copy link
Contributor Author

AtofStryker commented Aug 22, 2024

@AtofStryker

I haven't actually understood why nx-cloud is getting in a mess. I did notice though that postinstall deduplication is only used for local runs, not for CI, so I'm wondering perhaps if this is making nx-cloud run in an inconsistent state? The logs that I can see don't show the level of detail to be able to work this out however.

const postInstallCommands = {
local: 'patch-package && yarn-deduplicate --strategy=highest && lerna run rebuild-better-sqlite3 --scope @packages/server && yarn build && yarn build-v8-snapshot-dev',
ci: 'patch-package && lerna run rebuild-better-sqlite3 --scope @packages/server',
}

@MikeMcC399 @jennifer-shehane That's what is odd. I don't actually think this is an issue with the nx-cloud package. It just has to do with nx-cloud using 6.2.1 of tar, which is semver compatible with ^6.1.15, which when deduped points all the lower semver compatible dependencies to 6.2.1 which causes issues with an older version of the npm package (which is on version 8. Nodejs 18+ uses 9+. npm 8 is for NodeJs 16). The npm packages under the hood are used by semantic-release which we are 5 major versions behind. What I haven't figured out is why the npm package is being called when running yarn workspace cypress dtslint, but something with the native cjs loader does not play nicely with the combo of older npm and the newer 6.2.1 tar package.

What we could try to do is update semantic-release and remove the resolution on nx-cloud to see if the issue comes back, but updating semantic-release could prove more challenging since we use the programmatic API extensively and its an ESM only package now on version 20+.

@AtofStryker AtofStryker merged commit 1c719bb into develop Aug 22, 2024
83 of 85 checks passed
@AtofStryker AtofStryker deleted the fix/yarn_lock branch August 22, 2024 14:12
@MikeMcC399
Copy link
Contributor

@AtofStryker

Thanks for your insights! I had no idea!

I cross-checked this PR locally after merge and the two tests I did were fine:

  1. There is no change to yarn.lock after running yarn
  2. yarn workspace cypress dtslint

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.

4 participants