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

refactor(xsnap): Cleanup build.js #9674

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

Conversation

gibson042
Copy link
Member

Ref #9614
Ref #9618

Description

Simplifies and documents functions. See individual commits for details.

Security Considerations

None.

Scaling Considerations

n/a

Documentation Considerations

Includes JSDoc improvements.

Testing Considerations

None.

Upgrade Considerations

n/a

@gibson042 gibson042 requested a review from mhofman July 9, 2024 22:53
@gibson042 gibson042 force-pushed the gibson-2024-07-xsnap-build-cleanup branch from 308f456 to c981c22 Compare February 11, 2025 02:37
@gibson042 gibson042 requested a review from a team as a code owner February 11, 2025 02:37
Copy link

cloudflare-workers-and-pages bot commented Feb 11, 2025

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9d42867
Status: ✅  Deploy successful!
Preview URL: https://ab14868c.agoric-sdk.pages.dev
Branch Preview URL: https://gibson-2024-07-xsnap-build-c.agoric-sdk.pages.dev

View logs

@gibson042 gibson042 force-pushed the gibson-2024-07-xsnap-build-cleanup branch from 5a8d5d8 to 7d8c2a6 Compare February 13, 2025 17:57
@gibson042 gibson042 force-pushed the gibson-2024-07-xsnap-build-cleanup branch from 7d8c2a6 to 9d42867 Compare February 13, 2025 21:04
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

I think this looks like a pure refactor but I'm not confident I'm following all the changes. My main concern is breakage in the case of yarn install of xsnap. I believe we have poor coverage of that case.

@gibson042
Copy link
Member Author

gibson042 commented Feb 14, 2025

I think this looks like a pure refactor but I'm not confident I'm following all the changes. My main concern is breakage in the case of yarn install of xsnap. I believe we have poor coverage of that case.

@mhofman I just stepped through everything to re-convince myself, and came away satisfied... is there a commit that I should break down further? The most intricate was 4d4674d , which breaks down like so:

Given hasBin, hasSource, hasGit, showEnv = args.includes('--show-env');, and isWorkingCopy = hasGit || (!hasSource && !hasBin);...

old main behaviorold main behavior
  • If showEnv, then
    1. Assert isWorkingCopy.
    2. updateSubmodules(showEnv, …).
    3. <implicit return>
  • If isWorkingCopy, then
    1. updateSubmodules(showEnv, …).
    2. Set hasSource = true;.
  • If hasSource, then
    1. force = await (!hasBin || isRejected(npm.run(…)));.
    2. buildXsnap(platform, force, …).
  • Else if not hasBin, then
    1. throw.
  1. If args.includes('--show-env'), then
    1. Assert isWorkingCopy.
    2. updateSubmodules(true, …).
    3. return.
  2. If isWorkingCopy, then
    1. updateSubmodules(false, …).

  3. If hasSource or isWorkingCopy, then
    1. force = await (!hasBin || isRejected(npm.run(…)));.
    2. buildXsnap(platform, force, …).
  4. Else if not hasBin, then
    1. throw.

@mhofman
Copy link
Member

mhofman commented Feb 14, 2025

The most intricate was 4d4674d , which breaks down like so

Yeah that was the one that I wasn't sure of the equivalence just starring at the code last night. With the added explanation and a fresh brain, I'll take another pass

@gibson042 gibson042 requested a review from mhofman February 17, 2025 17:15
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.

3 participants