Skip to content

feat!(CLI): make contract optimize optional#672

Merged
paulbellamy merged 8 commits intostellar:mainfrom
theahaco:feat!/make_opt_opt_in
Jun 8, 2023
Merged

feat!(CLI): make contract optimize optional#672
paulbellamy merged 8 commits intostellar:mainfrom
theahaco:feat!/make_opt_opt_in

Conversation

@willemneal
Copy link
Copy Markdown
Contributor

What

Remove wasm-opt by default

Why

Some users had issues with building wasm-opt and this also makes installing it from source faster.

Known limitations

[TODO or N/A]

Copy link
Copy Markdown
Member

@leighmcculloch leighmcculloch 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 makes sense. I've seen folks struggle to install it from source too because of the need for a C compiler.

There's some logistical stuff we need to do to support this. See inline.

Comment thread cmd/soroban-cli/src/commands/contract/optimize.rs Outdated
Comment thread cmd/soroban-cli/Cargo.toml Outdated
Comment thread cmd/soroban-cli/Cargo.toml Outdated
@willemneal
Copy link
Copy Markdown
Contributor Author

@leighmcculloch Ideally this would be a plugin. This prevents the feature issues altogether.

However, currently we can only do soroban-x for plugins, but we should be able to add it to any subcommand e.g. soroban-contract-optimize. Then we have a stub that prompts the user to install the plugin.

The correctness of the rest of the CLI is independent from the optimize command, which is well tested underneath. Thanks @brson!

For now can we feature gate it and then return to pull it out before the next release so we can publish the plugin.

@willemneal willemneal force-pushed the feat!/make_opt_opt_in branch from fe84992 to fa07c00 Compare June 1, 2023 14:03
@leighmcculloch
Copy link
Copy Markdown
Member

leighmcculloch commented Jun 1, 2023

Ideally this would be a plugin. This prevents the feature issues altogether.

However, currently we can only do soroban-x for plugins, but we should be able to add it to any subcommand e.g. soroban-contract-optimize. Then we have a stub that prompts the user to install the plugin.

I don't think we need too be too precious on location. We can make it a plugin and put that plugin at the top level. We can move it later if we think plugins should be multi level, but multi level plugins may represent a small security concern (unexpected, surprise, impersonation, etc), so we should consider them carefully.

@willemneal
Copy link
Copy Markdown
Contributor Author

@leighmcculloch fair enough. Is this good enough for now and we move to a top level plugin later?

@leighmcculloch
Copy link
Copy Markdown
Member

If this is a short term gap, 👍🏻. Making this a plugin seems like the right move since the CLI has that capability. It's the perfect use case. Rust features are painful on a few fronts, and we've started trying to eliminate features from other repos. I think we'd want to eliminate this feature as soon as possible.

Comment thread cmd/soroban-cli/Cargo.toml Outdated
Copy link
Copy Markdown
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

I pushed a couple tweaks updating docs, etc. Looks good!

@leighmcculloch leighmcculloch enabled auto-merge (squash) June 1, 2023 20:02
@leighmcculloch
Copy link
Copy Markdown
Member

To be followed up by:

auto-merge was automatically disabled June 7, 2023 16:01

Head branch was pushed to by a user without write access

@willemneal
Copy link
Copy Markdown
Contributor Author

Looks like preflight is failing to link on windows:

c:/programdata/chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: D:/a/soroban-tools/soroban-tools/cmd/soroban-rpc/internal/preflight/../../../../target/x86_64-pc-windows-gnu/release-with-panic-unwind/\libpreflight.a(std-ca5208825e97b4ba.std.687851ba-cgu.0.rcgu.o): in function `std::sys::windows::fs::open_link_no_reparse':
/rustc/90c541806f23a127002de5b4038be731ba1458ca/library\std\src\sys\windows/fs.rs:800: undefined reference to `NtCreateFile'
c:/programdata/chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: /rustc/90c541806f23a127002de5b4038be731ba1458ca/library\std\src\sys\windows/fs.rs:829: undefined reference to `RtlNtStatusToDosError'
c:/programdata/chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: D:/a/soroban-tools/soroban-tools/cmd/soroban-rpc/internal/preflight/../../../../target/x86_64-pc-windows-gnu/release-with-panic-unwind/\libpreflight.a(std-ca5208825e97b4ba.std.687851ba-cgu.0.rcgu.o): in function `std::sys::windows::handle::Handle::synchronous_read':
/rustc/90c541806f23a127002de5b4038be731ba1458ca/library\std\src\sys\windows/handle.rs:241: undefined reference to `NtReadFile'
c:/programdata/chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: /rustc/90c541806f23a127002de5b4038be731ba1458ca/library\std\src\sys\windows/handle.rs:272: undefined reference to `RtlNtStatusToDosError'
c:/programdata/chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: D:/a/soroban-tools/soroban-tools/cmd/soroban-rpc/internal/preflight/../../../../target/x86_64-pc-windows-gnu/release-with-panic-unwind/\libpreflight.a(std-ca5208825e97b4ba.std.687851ba-cgu.0.rcgu.o): in function `std::sys::windows::handle::Handle::synchronous_write':
/rustc/90c541806f23a127002de5b4038be731ba1458ca/library\std\src\sys\windows/handle.rs:290: undefined reference to `NtWriteFile'
c:/programdata/chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: /rustc/90c541806f23a127002de5b4038be731ba1458ca/library\std\src\sys\windows/handle.rs:318: undefined reference to `RtlNtStatusToDosError'
collect2.exe: error: ld returned 1 exit status

@paulbellamy
Copy link
Copy Markdown
Contributor

Windows build fail seems due to github updating the windows image.

@paulbellamy paulbellamy enabled auto-merge (squash) June 7, 2023 18:59
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