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

CLI bundle: add debug and kernel options. fixes #1447 #1597

Merged
merged 12 commits into from
Dec 16, 2024
Merged

Conversation

paracetamolo
Copy link

@paracetamolo paracetamolo commented Dec 10, 2024

Closes #1447

Improve the bundle command:

  • add an explicit --debug option, which is otherwise false by default
  • add a --kernel option to pass the main kernel file and treat the directory as the kernel namespace library.

Also enforces that any library, kernel or not, has at least one export.

Note: we can still build a library w/o exports using from_dir which does not use Library::new.

@paracetamolo paracetamolo requested a review from plafer December 10, 2024 13:57
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

Thank you for taking this one on! Left a few suggestions

miden/src/cli/bundle.rs Outdated Show resolved Hide resolved
miden/src/cli/bundle.rs Outdated Show resolved Hide resolved
miden/src/cli/bundle.rs Outdated Show resolved Hide resolved
miden/src/cli/bundle.rs Outdated Show resolved Hide resolved
miden/tests/integration/cli/cli_test.rs Outdated Show resolved Hide resolved
miden/tests/integration/cli/cli_test.rs Show resolved Hide resolved
@plafer
Copy link
Contributor

plafer commented Dec 10, 2024

Also, to get the changelog CI to pass, you need to add an entry to CHANGELOG.md

@paracetamolo
Copy link
Author

The command also includes the stdlib now (for both kernel and normal libraries). This was necessary to bundle api.masm. Tested with:

cargo run --features executable -- bundle ~/miden/miden-base/miden-lib/asm/kernels/transaction/lib --kernel ~/miden/miden-base/miden-lib/asm/kernels/transaction/api.masm

Not sure if the test could be added to miden-base.

Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

Just a quick pass

miden/src/cli/bundle.rs Outdated Show resolved Hide resolved
miden/src/cli/bundle.rs Outdated Show resolved Hide resolved
miden/tests/integration/cli/cli_test.rs Outdated Show resolved Hide resolved
tested with:
cargo run --features executable -- bundle ~/miden/miden-base/miden-lib/asm/kernels/transaction/lib --kernel ~/miden/miden-base/miden-lib/asm/kernels/transaction/api.masm
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

LGTM!

@plafer plafer merged commit 21da03d into next Dec 16, 2024
9 checks passed
@plafer plafer deleted the marco-bundle branch December 16, 2024 22:09
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.

2 participants