build: no-bail when testing/linting whole repo #4646

turadg · 2022-02-23T14:51:26Z

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.

turadg · 2022-02-23T14:52:05Z

package.json

@@ -41,20 +41,20 @@
    "singleQuote": true
  },
  "scripts": {
-    "OFF-clean": "yarn workspaces run clean",
+    "clean": "yarn lerna run --no-bail clean",


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?

Figured this out. It was an infinite loop due to Lerna running this clean command again after the children.

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

michaelfig

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!

michaelfig · 2022-02-23T16:07:52Z

package.json

-    "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",


Suggested change

"test": "yarn lerna run --no-bail lint",

"test": "yarn lerna run --no-bail test",

warner · 2022-02-23T17:26:07Z

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.

turadg · 2022-02-23T17:26:47Z

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.

What were the symptoms? I noticed yarn clean had an infinite loop which I think is addressed with 1aa55aa

You're welcome to explore this solution, but be aware, here be dragons!

What would give you confidence to approve the PR?

warner

@michaelfig knows more about the concerns with lerna than me, but if he's good with it, I'm good with it.

michaelfig · 2022-02-23T20:13:40Z

Does the github CI viewer know how to find the errors this emits if it doesn't fail fast?

Only the lint CI output will be structured any differently. And that needs a scroll-through to find the multiple errors, but at least they won't be intermingled with test output.

turadg · 2022-02-23T20:16:45Z

@kriskowal I heard you might have some thoughts/plans for the Lerna update.

michaelfig · 2022-02-23T20:17:12Z

What would give you confidence to approve the PR?

Passing existing CI tests, then a once-over would be enough. Can you ping me for a review when CI is happy?

mhofman · 2022-02-23T20:29:38Z

The yarn.lock churn is high on this one! Are these really all due to the bump of lerna version? Put another way, does removing lerna from dependencies really only result in removal from the yarn.lock (which unfortunately also translates into changed lines for package entries with multiple matching semver).

turadg · 2022-02-23T20:51:04Z

The yarn.lock churn is high on this one! Are these really all due to the bump of lerna version?

Not sure I understand the hypothesis. I can tell you I changed the lerna version specifier and then ran yarn install.

Put another way, does removing lerna from dependencies really only result in removal from the yarn.lock (which unfortunately also translates into changed lines for package entries with multiple matching semver).

All removals, yes, though as you acknowledge some version removals translate into changed lines.

Well except for the mysterious change in resolution,

-ast-types@agoric-labs/ast-types#Agoric-built:
+"ast-types@github:agoric-labs/ast-types#Agoric-built":

Maybe due to how Agoric-built is being resolved? agoric-labs/recast@d9316d0
@michaelfig could we do this with patch-package instead of forking the repo?

mhofman · 2022-02-23T21:52:46Z

package.json

@@ -70,6 +70,7 @@
    "patch-package": "^6.2.2"
  },
  "resolutions": {
+    "**/ast-types": "github:agoric-labs/ast-types#Agoric-built",


I'd like to understand why this is necessary now, and wasn't necessary before?

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?

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"

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 of yarn install --frozen-lockfile?

Apparently that doesn't result in a non-zero exit code if it fails...

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.

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.

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 the if 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.

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 from yarn.lock.

run yarn install --frozen-lockfile. It doesn't put it back (check the git 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.

kriskowal · 2022-02-23T21:56:29Z

@kriskowal I heard you might have some thoughts/plans for the Lerna update.

I don’t. Michael’s our resident Lerna expert and I only know enough to do releases from Endo.

michaelfig

Looking good. Just one more question from me.

michaelfig · 2022-02-24T16:47:19Z

patches/@lerna+conventional-commits+3.18.5.dev.patch

-        const releaseType = data.releaseType || "patch";
-+        let releaseType = data.releaseType || "patch";
-+        
-+        // Don't gratuitously break compatibility with clients using `^0.x.y`.


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.

Yes, I mentioned this in e78169b but should have mentioned in PR too:

it includes what we were patching: lerna/lerna#2486

Vindication!

kriskowal · 2022-02-24T16:52:36Z

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.

turadg · 2022-02-24T17:09:20Z

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 gif diff --exit-code should be moved to after yarn build?

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)

turadg requested review from warner and michaelfig February 23, 2022 14:51

turadg commented Feb 23, 2022

View reviewed changes

michaelfig reviewed Feb 23, 2022

View reviewed changes

warner approved these changes Feb 23, 2022

View reviewed changes

mhofman reviewed Feb 23, 2022

View reviewed changes

turadg force-pushed the ta/lint-all-packages branch 2 times, most recently from b8efb09 to dc90006 Compare February 24, 2022 16:32

turadg requested a review from michaelfig February 24, 2022 16:40

michaelfig reviewed Feb 24, 2022

View reviewed changes

turadg mentioned this pull request Feb 24, 2022

remove Lerna #4660

Open

turadg force-pushed the ta/lint-all-packages branch from dc90006 to 793204b Compare February 24, 2022 17:11

turadg changed the title ~~chore(root): no-bail when running commands over each workspace~~ chore(root): no-bail when testing/linting whole repo Feb 24, 2022

turadg changed the title ~~chore(root): no-bail when testing/linting whole repo~~ build: no-bail when testing/linting whole repo Feb 24, 2022

turadg added the automerge:no-update (expert!) Automatically merge without updates label Feb 24, 2022

turadg added 4 commits February 24, 2022 09:23

chore(root): no-bail when testing/linting whole repo

6a25e98

build: show changes in git diff check

6f9e9a5

chore(sdk): update yarn

632abbb

build: monorepo root not a package

958ffaa

turadg force-pushed the ta/lint-all-packages branch from 793204b to 958ffaa Compare February 24, 2022 17:27

mergify bot merged commit c8cc6c2 into master Feb 24, 2022

mergify bot deleted the ta/lint-all-packages branch February 24, 2022 17:44

This was referenced Nov 30, 2023

[Snyk] Security upgrade @agoric/cosmic-swingset from 0.19.0 to 0.34.6 Kashuhackerone/agoric-sdk#220

Open

[Snyk] Fix for 1 vulnerabilities Kashuhackerone/agoric-sdk#240

Open

Kashuhackerone mentioned this pull request Dec 31, 2023

[Snyk] Security upgrade @agoric/cosmic-swingset from 0.19.0 to 0.34.6 Kashuhackerone/agoric-sdk#246

Open

Kashuhackerone mentioned this pull request Jan 20, 2024

[Snyk] Security upgrade @agoric/swingset-vat from 0.10.0 to 0.25.1 Kashuhackerone/agoric-sdk#252

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: no-bail when testing/linting whole repo #4646

build: no-bail when testing/linting whole repo #4646

turadg commented Feb 23, 2022

turadg Feb 23, 2022 •

edited

turadg Feb 23, 2022

turadg Feb 23, 2022 •

edited

michaelfig left a comment

michaelfig Feb 23, 2022

warner commented Feb 23, 2022

turadg commented Feb 23, 2022

warner left a comment

michaelfig commented Feb 23, 2022

turadg commented Feb 23, 2022

michaelfig commented Feb 23, 2022

mhofman commented Feb 23, 2022

turadg commented Feb 23, 2022

mhofman Feb 23, 2022

kriskowal Feb 23, 2022

turadg Feb 23, 2022

mhofman Feb 23, 2022

turadg Feb 24, 2022

michaelfig Feb 24, 2022

turadg Feb 24, 2022

michaelfig Feb 24, 2022

kriskowal commented Feb 23, 2022

michaelfig left a comment

michaelfig Feb 24, 2022

turadg Feb 24, 2022

kriskowal Feb 24, 2022

kriskowal commented Feb 24, 2022

turadg commented Feb 24, 2022

	"test": "yarn lerna run --no-bail lint",
	"test": "yarn lerna run --no-bail test",

build: no-bail when testing/linting whole repo #4646

build: no-bail when testing/linting whole repo #4646

Conversation

turadg commented Feb 23, 2022

Description

Before

After

Security Considerations

Documentation Considerations

Testing Considerations

turadg Feb 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Feb 23, 2022 • edited

Choose a reason for hiding this comment

michaelfig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warner commented Feb 23, 2022

turadg commented Feb 23, 2022

warner left a comment

Choose a reason for hiding this comment

michaelfig commented Feb 23, 2022

turadg commented Feb 23, 2022

michaelfig commented Feb 23, 2022

mhofman commented Feb 23, 2022

turadg commented Feb 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented Feb 23, 2022

michaelfig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriskowal commented Feb 24, 2022

turadg commented Feb 24, 2022

turadg Feb 23, 2022 •

edited

turadg Feb 23, 2022 •

edited