-
Notifications
You must be signed in to change notification settings - Fork 206
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
build: no-bail when testing/linting whole repo #4646
Conversation
package.json
Outdated
@@ -41,20 +41,20 @@ | |||
"singleQuote": true | |||
}, | |||
"scripts": { | |||
"OFF-clean": "yarn workspaces run clean", | |||
"clean": "yarn lerna run --no-bail clean", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this also restores the yarn clean
command. I thought it was disabled because not all packages have it, but when I run locally it's hanging after > rm -rf xsnap-native/xsnap/build
. Any ideas why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured this out. It was an infinite loop due to Lerna running this clean command again after the children.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the reason is that we had .
(root) listed as a package in lerna.json. @michaelfig could that be the dragon?
c71baaf fixes that and goes ahead to DRY out the list of packages using https://github.com/lerna/lerna/blob/main/commands/bootstrap/README.md#--use-workspaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do note, I wasn't able to get yarn lerna run ...
working consistently in the past. One problem I ran into was that the default concurrency settings may have caused problems.
You're welcome to explore this solution, but be aware, here be dragons!
package.json
Outdated
"lint:packages": "yarn workspaces run lint", | ||
"test": "yarn workspaces run test", | ||
"lint:packages": "yarn lerna run --no-bail lint", | ||
"test": "yarn lerna run --no-bail lint", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"test": "yarn lerna run --no-bail lint", | |
"test": "yarn lerna run --no-bail test", |
Does the github CI viewer know how to find the errors this emits if it doesn't fail fast? I wouldn't want to scroll through the mountains of logged-exception noise in our test output to find the one part that failed. |
What were the symptoms? I noticed
What would give you confidence to approve the PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelfig knows more about the concerns with lerna
than me, but if he's good with it, I'm good with it.
Only the |
@kriskowal I heard you might have some thoughts/plans for the Lerna update. |
Passing existing CI tests, then a once-over would be enough. Can you ping me for a review when CI is happy? |
The |
Not sure I understand the hypothesis. I can tell you I changed the
All removals, yes, though as you acknowledge some version removals translate into changed lines. Well except for the mysterious change in resolution,
Maybe due to how |
package.json
Outdated
@@ -70,6 +70,7 @@ | |||
"patch-package": "^6.2.2" | |||
}, | |||
"resolutions": { | |||
"**/ast-types": "github:agoric-labs/ast-types#Agoric-built", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to understand why this is necessary now, and wasn't necessary before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious as well. ast-types
is brought in via recast
via @endo/static-module-record
via @endo/compartment-mapper
via bundle utilities. We had to modify it to make it work under SES. It can’t be a patch package because the patch would need to be distributed to dapps and any other dependee transitively. We may need to also patch ast-types
so it runs under -r esm
if we end up spending a lot of time supporting contract authors who stub their toe on ast-types does not have a default export
errors often enough or if rolling forward is hard enough.
There shouldn’t be a need for ast-types
to be deduplicated. Perhaps there’s another dependency path that brings in the wrong ast-types
and yarn
is deduping them to our detriment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also like to know! This resolution is just to make my laptop write out the same yarn.lock that CI does. Incidentally, why do we have a git diff
check instead of yarn install --frozen-lockfile
?
Thanks for pointing out why it can't be a patch. How about a versioned release?
There shouldn’t be a need for ast-types to be deduplicated. Perhaps there’s another dependency path that brings in the wrong ast-types and yarn is deduping them to our detriment?
That would make sense, but there's just one thing depending on ast-types
:
recast@agoric-labs/recast#Agoric-built:
version "0.20.5"
resolved "https://codeload.github.com/agoric-labs/recast/tar.gz/879398a55cd50a53ade179de203706a25c53fb49"
dependencies:
ast-types "github:agoric-labs/ast-types#Agoric-built"
esprima "~4.0.0"
source-map "~0.6.1"
tslib "^2.0.1"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a versioned release?
That was suggested be @kriskowal as an alternative. Given the release complexity we ran into with github, and now this, we should seriously consider this again.
Incidentally, why do we have a
git diff
check instead ofyarn install --frozen-lockfile
?
Apparently that doesn't result in a non-zero exit code if it fails...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently that doesn't result in a non-zero exit code if it fails...
I've heard rumors of this but not witnessed it. The docs say it does and a test locally does,
It's also what I used for the past several years without incident.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't object to your questioning my judgement. But I don't want to be the one to have to repeat diagnosing the problem I ran into where yarn --frozen-lockfile
didn't fail with an error but the lockfile still differed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing personal here. My last comment above was in response to the assertion "doesn't result in a non-zero exit code if it fails".
My questioning whether we really need to have our own diff check instead of frozen-lock
preceded your answer in chat,
In the testing I did around this, the changes I was able to induce without --frozen-lockfile failing were (material) semantic ones.
That information leads me to a different questions:
- what is a way to reproduce that bug in frozen-lockfile? Not because I doubt that you encountered it, but because 1) it might have been fixed in yarn since then and 2) if it hasn't I'd like to file it as a bug
- whether there are downsides to using
--frozen-lockfile
anyway, to convey that intent (keeping theif git is dirty
as a backup for this bug)
In the interests of reducing work in flight I'll make that change optimistically and request a code review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing personal here.
No worries! I was being unnecessarily short.
My last comment above was in response to the assertion "doesn't result in a non-zero exit code if it fails".
Steps to reproduce:
- delete, say, the
@agoric/nat^4.1.0
section fromyarn.lock
. - run
yarn install --frozen-lockfile
. It doesn't put it back (check thegit diff
), but it also doesn't error. - run
yarn install
, it puts the@agoric/nat^4.1.0
section back
So the issue I found was that --frozen-lockfile
doesn't complain about the things that a normal yarn install
would have changed. I'm more interested in making sure that yarn install
wouldn't have changed anything, not just that the yarn.lock
isn't modified.
I don’t. Michael’s our resident Lerna expert and I only know enough to do releases from Endo. |
b8efb09
to
dc90006
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Just one more question from me.
- const releaseType = data.releaseType || "patch"; | ||
+ let releaseType = data.releaseType || "patch"; | ||
+ | ||
+ // Don't gratuitously break compatibility with clients using `^0.x.y`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this logic in lerna@4.0.0
? We needed it so that our pre-1.x.x
packages didn't get bumped up to 1.x.x
just because they had a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I mentioned this in e78169b but should have mentioned in PR too:
it includes what we were patching: lerna/lerna#2486
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vindication!
We also want to be sure that no committed files change after yarn build. There is one generated file in agoric-cli that changes if we fail to run yarn build and commit too. |
Does that mean the Either way I want to punt the changes in Lerna per se to #4660 so I can get this more immediate pain point solved (CI doesn't show all lint/test errors) |
dc90006
to
793204b
Compare
793204b
to
958ffaa
Compare
Description
The errors CI reports in linting aren't exhaustive because it fails-fast. #4416 fixed this within package, but the workspace iterator still stops when an error is encountered (and linting failure necessarily returns a non-zero exit code).
The change here is to use https://npm.io/package/@lerna/run to use it's
--no-bail
option. (Yarn "classic" and "berry" do no support such an option).Demonstrating with some hardcoded failures:
Before
After
The latter is more verbose because the default log level is "info". The next log level down looks like the following, but I thought it would be better to keep the output of time spent.
You might also notice the warning about cycles, filed here: #4645
Security Considerations
None.
Documentation Considerations
Works now more as expected.
Testing Considerations
Improved.