Disable `--test` flag when publishing package using Sui client #13786

rockbmb · 2023-09-14T16:56:52Z

Description

This PR disables the use of the --test flag when executing sui client publish.
It closes #13679, following discussion in that thread.

Test Plan

I've added a test to ./crates/sui/tests/cli_tests.rs that verifies the --test flag will no longer be accepted with client publish, along with an error message explaining why.

Type of Change (Check all that apply)

protocol change
user-visible impact
breaking change for a client SDKs
breaking change for FNs (FN binary must upgrade)
breaking change for validators or node operators (must upgrade binaries)
breaking change for on-chain data layout
necessitate either a data wipe or data migration

Release notes

The Sui CLI now returns a new error message if you try to compile and publish a package in test mode using the sui client publish --test command.

vercel · 2023-09-14T16:56:58Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
mysten-ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 16, 2023 0:02am
sui-typescript-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 16, 2023 0:02am

3 Ignored Deployments

Name	Status	Preview	Updated (UTC)
explorer	⬜️ Ignored (Inspect)	Visit Preview	Sep 16, 2023 0:02am
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Sep 16, 2023 0:02am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Sep 16, 2023 0:02am

rockbmb · 2023-09-14T17:31:03Z

@tzakian Hey, two questions

I can't add you as a reviewer, could you kindly do so for me, please?
Regarding the Release notes section of the PR description I have to fill out, is there anything in particular I should write?

Thanks!

tzakian · 2023-09-14T19:10:04Z

Thank you for the PR @rockbmb! I will take a look at this and may add some other reviewers as well :)

In terms of release notes just describing what this prevents is good enough (and will get picked up). E.g., "Added error to Sui CLI when trying to publish a package compiled in test mode"

rockbmb · 2023-09-14T23:46:54Z

crates/sui/src/client_commands.rs

@@ -732,6 +732,17 @@ impl SuiClientCommands {
                serialize_signed_transaction,
                lint,
            } => {
+                if build_config.test_mode {


While the solution presented in this PR works, a user running sui client publish --help will still see --test in the list of available flags.

In order to remove this entirely, I can see two solutions:

BuildConfig is duplicated into a struct without a test_mode field, and used here

The test_mode flag is extracted into a different structure BuildConfigNoTest that two fields: it, and athe original BuildConfig

Both feel like overengineering, so I chose this approach instead.

This makes sense to me as well -- agreed that the other options feel a bit like overengineering as well.

tzakian

Thanks for the PR (and thank you for the additional test too!).

Only minor nit is on the exact wording of the error message and removing the example, but otherwise this looks great to me! Once that's fixed I think this can go in.

tzakian · 2023-09-15T22:17:04Z

crates/sui/src/client_commands.rs

@@ -732,6 +732,17 @@ impl SuiClientCommands {
                serialize_signed_transaction,
                lint,
            } => {
+                if build_config.test_mode {


This makes sense to me as well -- agreed that the other options feel a bit like overengineering as well.

tzakian · 2023-09-15T22:17:37Z

crates/sui/src/client_commands.rs

+                    return Err(SuiError::ModulePublishFailure {
+                        error: "The `publish` subcommand should not be used with the `--test` flag\n\
+                                \n\
+                                Library code in published packages must not depend on test code.\n\


Nit:

Suggested change

Library code in published packages must not depend on test code.\n\

Code in published packages must not depend on test code.\n\

tzakian · 2023-09-15T22:26:45Z

crates/sui/src/client_commands.rs

+                                In order to fix this and publish the package without `--test`, search for, and remove \
+                                instances of e.g. test modules declared as `friend`s of modules from `sources/`.".to_string(),


I would most likely rephrase the last part of this a bit as the way tests can be defined (and how modules could depend on other test-only code) can differ, e.g., you can have test-only modules under sources/, and even though this is an example at the end I worry it may put blinders on folks and how they look at fixing this issue.

What do you think of maybe something like:

In order to fix this and publish the package without `--test` remove any non-test dependencies on test-only code. You can ensure all test-only dependencies have been removed by compiling the package normally with `sui move build`.

Sounds good to me.

tzakian

This looks good to me. Thank you for the fix, and following up with the changes!

vercel bot deployed to Preview – sui-typescript-docs September 14, 2023 16:57 View deployment

vercel bot deployed to Preview – mysten-ui September 14, 2023 16:57 View deployment

rockbmb force-pushed the sui-client-publish-test-flag branch from 49d2d5f to fb583fe Compare September 14, 2023 17:00

vercel bot deployed to Preview – mysten-ui September 14, 2023 17:01 View deployment

vercel bot deployed to Preview – sui-typescript-docs September 14, 2023 17:02 View deployment

rockbmb added 2 commits September 14, 2023 18:20

Disable --test flag with sui client publish

4a60322

Add test to disabled --test flag in sui client publish

338e18c

rockbmb force-pushed the sui-client-publish-test-flag branch from fb583fe to 338e18c Compare September 14, 2023 17:21

vercel bot deployed to Preview – mysten-ui September 14, 2023 17:22 View deployment

vercel bot deployed to Preview – sui-typescript-docs September 14, 2023 17:22 View deployment

tzakian requested review from amnn, rvantonder, patrickkuo, stefan-mysten and tzakian September 14, 2023 19:10

rockbmb commented Sep 14, 2023

View reviewed changes

tzakian reviewed Sep 15, 2023

View reviewed changes

vercel bot deployed to Preview – mysten-ui September 15, 2023 23:51 View deployment

vercel bot deployed to Preview – sui-typescript-docs September 15, 2023 23:51 View deployment

rockbmb force-pushed the sui-client-publish-test-flag branch from 7620a8b to 420a4a6 Compare September 15, 2023 23:58

vercel bot deployed to Preview – sui-typescript-docs September 15, 2023 23:58 View deployment

vercel bot deployed to Preview – mysten-ui September 15, 2023 23:58 View deployment

Perform fixes from PR review

8b388e9

rockbmb force-pushed the sui-client-publish-test-flag branch from 420a4a6 to 8b388e9 Compare September 16, 2023 00:02

vercel bot deployed to Preview – sui-typescript-docs September 16, 2023 00:02 View deployment

vercel bot deployed to Preview – mysten-ui September 16, 2023 00:02 View deployment

rockbmb requested a review from tzakian September 18, 2023 10:54

tzakian approved these changes Sep 18, 2023

View reviewed changes

tzakian merged commit 0041229 into MystenLabs:main Sep 18, 2023
36 checks passed

rockbmb deleted the sui-client-publish-test-flag branch September 18, 2023 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable `--test` flag when publishing package using Sui client #13786

Disable `--test` flag when publishing package using Sui client #13786

rockbmb commented Sep 14, 2023 •

edited by randall-Mysten

Loading

vercel bot commented Sep 14, 2023 •

edited

Loading

rockbmb commented Sep 14, 2023

tzakian commented Sep 14, 2023

rockbmb Sep 14, 2023

tzakian Sep 15, 2023

tzakian left a comment

tzakian Sep 15, 2023

tzakian Sep 15, 2023

tzakian Sep 15, 2023

rockbmb Sep 15, 2023

tzakian left a comment

	Library code in published packages must not depend on test code.\n\
	Code in published packages must not depend on test code.\n\

		In order to fix this and publish the package without `--test`, search for, and remove \
		instances of e.g. test modules declared as `friend`s of modules from `sources/`.".to_string(),

Disable --test flag when publishing package using Sui client #13786

Disable --test flag when publishing package using Sui client #13786

Conversation

rockbmb commented Sep 14, 2023 • edited by randall-Mysten Loading

Description

Test Plan

Type of Change (Check all that apply)

Release notes

vercel bot commented Sep 14, 2023 • edited Loading

rockbmb commented Sep 14, 2023

tzakian commented Sep 14, 2023

rockbmb Sep 14, 2023

Choose a reason for hiding this comment

tzakian Sep 15, 2023

Choose a reason for hiding this comment

tzakian left a comment

Choose a reason for hiding this comment

tzakian Sep 15, 2023

Choose a reason for hiding this comment

tzakian Sep 15, 2023

Choose a reason for hiding this comment

tzakian Sep 15, 2023

Choose a reason for hiding this comment

rockbmb Sep 15, 2023

Choose a reason for hiding this comment

tzakian left a comment

Choose a reason for hiding this comment

Disable `--test` flag when publishing package using Sui client #13786

Disable `--test` flag when publishing package using Sui client #13786

rockbmb commented Sep 14, 2023 •

edited by randall-Mysten

Loading

vercel bot commented Sep 14, 2023 •

edited

Loading