Skip to content

Add a target option for simc - #364

Open
ivanlele wants to merge 1 commit into
BlockstreamResearch:masterfrom
ivanlele:feature/target-cli-option
Open

Add a target option for simc#364
ivanlele wants to merge 1 commit into
BlockstreamResearch:masterfrom
ivanlele:feature/target-cli-option

Conversation

@ivanlele

@ivanlele ivanlele commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

This PR creates a new compilation option to choose a target jet set. Closes #224

@ivanlele
ivanlele requested a review from delta1 as a code owner June 25, 2026 11:30
@ivanlele
ivanlele force-pushed the feature/target-cli-option branch 2 times, most recently from cc8d942 to 416a8df Compare June 25, 2026 11:44
@KyrylR

KyrylR commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

needs rebase

@apoelstra

apoelstra commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

416a8df needs rebase

@ivanlele
ivanlele force-pushed the feature/target-cli-option branch from 416a8df to 5d57119 Compare June 29, 2026 09:54
@ivanlele

ivanlele commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Rebased

@ivanlele
ivanlele force-pushed the feature/target-cli-option branch from 5d57119 to 07ce8ba Compare July 3, 2026 09:31
@ivanlele

ivanlele commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

Made some improvements per @KyrylR's suggestions

@ivanlele
ivanlele force-pushed the feature/target-cli-option branch from 07ce8ba to 235a814 Compare July 3, 2026 10:07
KyrylR
KyrylR approved these changes Jul 3, 2026
View reviewed changes

@KyrylR KyrylR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ACK 235a814; successfully ran local tests, code review

@KyrylR

KyrylR commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

needs rebase

@ivanlele
ivanlele force-pushed the feature/target-cli-option branch from 235a814 to ad3a9e8 Compare July 16, 2026 13:45
@KyrylR

KyrylR commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

cc @delta1

@ivanlele needs rebase again

@ivanlele
ivanlele force-pushed the feature/target-cli-option branch from ad3a9e8 to c0772e0 Compare August 31, 2026 14:55
apoelstra
apoelstra reviewed Sep 1, 2026
View reviewed changes
Comment thread src/main.rs Outdated
@ivanlele
ivanlele force-pushed the feature/target-cli-option branch 2 times, most recently from 7172f08 to 667d0b3 Compare September 2, 2026 09:25
stringhandler
stringhandler reviewed Sep 2, 2026
View reviewed changes

@stringhandler stringhandler left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the feature gating is ok, but I would say that we should always print the options in clap, but rather error if they are used without the correct compiled features.

Some other LLM findings:

Output records compiler_version but not the target, and the target changes the CMR

The doc comment on compiler_version at line 30 gives the rationale for carrying it: "Different compiler versions can produce different CMRs from the same source, so the version travels with the artifact as metadata."

That reasoning now applies verbatim to --target. Same source, same compiler, different target:

$ simc examples/cat.simf # CMR e65e19e139a13583a0a7efb24be13c20d578f06f51b2a7fe7c7b9097072dbabe
$ simc -t core examples/cat.simf # CMR c83aea0e102548c2a441c6d4d268fb0469263601ef705ea61fa18882938ec06f

With --json consumers now unable to tell which jet set an artifact was built against, a target: String field on Output seems like it belongs in this PR. (It would need Display/Debug on Target, which it doesn't currently derive.)

Comment thread src/main.rs Outdated
Comment thread src/main.rs
Comment thread src/main.rs Outdated
@ivanlele
ivanlele force-pushed the feature/target-cli-option branch from 667d0b3 to fc4f030 Compare September 3, 2026 11:21
@ivanlele

ivanlele commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

I iterated on all comments that been written here

@ivanlele
ivanlele force-pushed the feature/target-cli-option branch from fc4f030 to 1320c9a Compare September 21, 2026 13:49
@apoelstra

apoelstra commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

1320c9a needs rebase

@ivanlele
ivanlele force-pushed the feature/target-cli-option branch from 1320c9a to b3297d2 Compare September 21, 2026 13:55
@ivanlele

ivanlele commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

1320c9a needs rebase

Done. I also tried to address your latest comment here

@stringhandler

stringhandler commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

NACK b3297d2

We can't realistically ever guarantee the safety of a dynamic path load. As described we can lower the scope of --target to just Elements and Core, or we can change it to only support WASM loaded jets via the command line

@ivanlele
ivanlele force-pushed the feature/target-cli-option branch 2 times, most recently from 3de1c8b to 82e458d Compare October 6, 2026 13:11
@ivanlele
ivanlele force-pushed the feature/target-cli-option branch from 82e458d to a44761c Compare October 6, 2026 14:57
@ivanlele

ivanlele commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

I updated this PR per Mike's suggestion

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apoelstra apoelstra apoelstra left review comments
@stringhandler stringhandler stringhandler left review comments
@KyrylR KyrylR KyrylR approved these changes
@delta1 delta1 Awaiting requested review from delta1 delta1 is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Overriding the Elements jet set with the --target parameter

4 participants

@ivanlele @KyrylR @apoelstra @stringhandler