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

fix(agoric-cli): Fully generalize package name extractor from zip files #9669

Merged
merged 2 commits into from
Jul 12, 2024

Conversation

kriskowal
Copy link
Member

Description

Following #9667 we learn that the bundle analyzer needs to understand exotic package names that include -dev$hash and such. To that end, this change fully generalizes the package name detector based on the @ prefix distinguishing a package name with two path components.

Security Considerations

None.

Scaling Considerations

None.

Documentation Considerations

None.

Testing Considerations

None.

Upgrade Considerations

None.

@kriskowal kriskowal requested a review from turadg July 9, 2024 01:02
Copy link

cloudflare-workers-and-pages bot commented Jul 9, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1de3fd4
Status: ✅  Deploy successful!
Preview URL: https://4d11db2f.agoric-sdk.pages.dev
Branch Preview URL: https://kriskowal-allow-bundle-trebs.agoric-sdk.pages.dev

View logs

@@ -11,7 +11,7 @@ import { ZipReader } from '@endo/zip';
/** @import {Bundle} from '@agoric/swingset-vat'; */
/** @import {CoreEvalPlan} from '@agoric/deploy-script-support/src/writeCoreEvalParts.js' */

const PACKAGE_NAME_RE = /(?<packageName>.*-v[\d.]+(?:-n\d+)?)\//;
Copy link
Member

Choose a reason for hiding this comment

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

I find the named capture groups more clear but won't block.

Consider a regression unit test for different name strings observed that have failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider a regression unit test for different name strings observed that have failed.

That's what I came back here to add. I'd be willing to add a commit for that if it would help.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added some tests. Feel free to add or subtract.

@kriskowal kriskowal added the automerge:rebase Automatically rebase updates, then merge label Jul 10, 2024
@kriskowal kriskowal force-pushed the kriskowal-allow-bundle-trebs branch from 1b38ceb to fa3c4d4 Compare July 10, 2024 20:50
@kriskowal kriskowal force-pushed the kriskowal-allow-bundle-trebs branch from fa3c4d4 to 1de3fd4 Compare July 12, 2024 21:34
@mergify mergify bot merged commit 7b85450 into master Jul 12, 2024
78 checks passed
@mergify mergify bot deleted the kriskowal-allow-bundle-trebs branch July 12, 2024 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants