Read compiler configurations from toml #1347

salaheldinsoliman · 2023-06-02T01:48:07Z

1- Implement taking solang's configurations from toml file. solang compile --configuration-file.
2- Implement solang new. a command that creates a new directory with a flipper example as well as the corresponding toml file. solang new --target substrate.

xermicus

Nice! Gave it a first review round. Looks quite good already

xermicus · 2023-06-05T07:40:04Z

examples/solana/solana_config.toml

+llvm-IR_optimization_level = "default"  # Set llvm optimizer level. Valid options are "none", "less", "default", "aggressive"
+
+[compiler-output]
+#verbose = false    # show debug messages


I don't think all of them need to be commented out

xermicus · 2023-06-05T07:40:13Z

examples/substrate/substrate_config.toml

+llvm-IR_optimization_level = "default"  # Set llvm optimizer level. Valid options are "none", "less", "default", "aggressive"
+
+[compiler-output]
+#verbose = false    # show debug messages


xermicus · 2023-06-05T07:41:07Z

src/bin/cli/mod.rs

@@ -35,6 +36,18 @@ pub enum Commands {

    #[command(about = "Generate Solidity interface files from Anchor IDL files")]
    Idl(IdlCommand),
+
+    #[command(about = "Create a new project with a ready example")]


Suggested change

#[command(about = "Create a new project with a ready example")]

#[command(about = "Create a new Solang project")]

xermicus · 2023-06-05T07:43:06Z

src/bin/cli/mod.rs

+    #[arg(name = "TARGETNAME",required= true, long = "target", value_parser = ["solana", "substrate", "evm"], help = "Target to build for [possible values: solana, substrate]", num_args = 1, hide_possible_values = true)]
+    pub target_name: String,
+
+    #[arg(name = "INPUT", help = "project name", num_args = 1, value_parser =  ValueParser::os_string())]


Suggested change

#[arg(name = "INPUT", help = "project name", num_args = 1, value_parser = ValueParser::os_string())]

#[arg(name = "INPUT", help = "Name of the project", num_args = 1, value_parser = ValueParser::os_string())]

xermicus · 2023-06-05T07:44:08Z

src/bin/cli/mod.rs

 pub struct Compile {
+    #[arg(name = "CONFFILE", help = "Take arguments from configuration file", long = "configuration-file", exclusive = true, value_parser = ValueParser::os_string(), required_unless_present = "INPUT", num_args = 0..=1, default_missing_value = "Solang.toml")]


I think it's better to no use a capital S in the default file

Suggested change

#[arg(name = "CONFFILE", help = "Take arguments from configuration file", long = "configuration-file", exclusive = true, value_parser = ValueParser::os_string(), required_unless_present = "INPUT", num_args = 0..=1, default_missing_value = "Solang.toml")]

#[arg(name = "CONFFILE", help = "Take arguments from configuration file", long = "configuration-file", exclusive = true, value_parser = ValueParser::os_string(), required_unless_present = "INPUT", num_args = 0..=1, default_missing_value = "solang.toml")]

xermicus · 2023-06-05T07:45:27Z

src/bin/cli/mod.rs

 pub struct Compile {
+    #[arg(name = "CONFFILE", help = "Take arguments from configuration file", long = "configuration-file", exclusive = true, value_parser = ValueParser::os_string(), required_unless_present = "INPUT", num_args = 0..=1, default_missing_value = "Solang.toml")]


Maybe we can just name this flag --config (or config-file) to keep it shorter?

xermicus · 2023-06-05T07:49:46Z

src/bin/cli/mod.rs

+                "ast-dot"|"cfg"|"llvm-ir"|"llvm-bc" |"object"| "asm" => {
+                    Ok(Some(value))
+                },
+                _ => {Err(serde::de::Error::custom("Invalid option for `emit`. Valid options are: `ast-dot`, `cfg`, `llvm-ir`, `llvm-bc`, `object`, `asm`"))}


This does not seem to be formatted correctly

src/bin/cli/test.rs

src/bin/solang.rs

LucasSte · 2023-06-05T13:29:18Z

src/bin/cli/mod.rs

+    let name = if let Some(name) = &target_arg.name {
+        name
+    } else {
+        eprintln!("error: must specifiy target name");


Suggested change

eprintln!("error: must specifiy target name");

eprintln!("error: compilation target not specified");

LucasSte · 2023-06-05T13:31:03Z

src/bin/cli/mod.rs

 }

 pub(crate) fn target_arg(target_arg: &TargetArg) -> Target {
-    if target_arg.name.as_str() == "solana" || target_arg.name.as_str() == "evm" {
+    let name = if let Some(name) = &target_arg.name {


I suggest calling this variable target_name.

LucasSte · 2023-06-05T13:32:05Z

src/bin/cli/mod.rs

+    }
+}
+
+fn emit_de<'de, D>(deserializer: D) -> Result<Option<String>, D::Error>


This function deserves a better name.

LucasSte · 2023-06-05T13:35:33Z

src/bin/cli/mod.rs

+fn default_true() -> bool {
+    true
+}


As per this PR iddm/serde-aux#23, you don't need this function. You could just write default_true.

for some reason, bool_true doesn't work

LucasSte · 2023-06-05T13:37:52Z

src/bin/cli/test.rs

+        package_toml = r#"
+        contracts = ["flipper"] # Contracts to include from the compiled files
+        import_path = ["path1", "path2"]   
+        import_map = ["map1=path", "map2=path2"]    # Maps to import. Define as : import_paths = ["map=path/to/map", "map2=path/to/map2", ..]"#;


Other commands like target, optimizations and emit could also be tested.

LucasSte · 2023-06-05T13:39:50Z

src/bin/cli/mod.rs

+}
+
+#[derive(Args)]
+pub struct New {


This struct deserves a better name. New is too generic.

I disagree on this one, I don't see how this could not be very obvious that it's the struct for the new subcommand, given the struct is in the cli module. Additionally I think it's confusing if the struct name diverges from the subcommand name

It might make sense here, but fn new(args: New) in line 70 from src/bin/solang.rs deserves either another name or a doc comment. I suggested a new name here because of this awkward construct.

Hmm yeah that's a good point, the new function should definitively be renamed.

Should this be called init, just like git init?

cargo allows cargo init and cargo new. git new does not exist

But there is cargo contract new
https://github.com/paritytech/cargo-contract#cargo-contract-new-my_contract

Signed-off-by: salaheldinsoliman <[email protected]>

xermicus

This is great work, thanks! ❤️

Can you please add a short section in the docs about this?

What readers might be curios about is what the default values look like. E.g. if you have an empty TOML or empty sections in the TOML.

Also my gut feeling is that many users want to depend on some openzepplin contracts, would be nice if the docs would show how to use importmaps in the TOML file.

src/bin/solang_new_examples/solana/flipper.sol

src/bin/solang_new_examples/substrate/flipper.sol

seanyoung · 2023-06-13T12:41:20Z

Cargo.toml

@@ -60,6 +60,7 @@ scale-info = "2.4"
 petgraph = "0.6.3"
 wasmparser = "0.106.0"
 wasm-encoder = "0.28"
+toml = "0.7.3"


0.7.4 is the latest. It might be better to use "0.7" because we are unlikely to care about the patch level

seanyoung · 2023-06-13T12:44:30Z

examples/solana/solana_config.toml

+input_files = ["flipper.sol"]   # Files to be compiled. You can define multiple files as : input_files = ["file1", "file2", ..]
+contracts = ["flipper"] # Contracts to include from the compiled files
+import_path = []   
+import_map = []    # Maps to import. Define as : import_paths = ["map=path/to/map", "map2=path/to/map2", ..]


This is an unnatural way of specifying a map in toml. How about

[package] [import_map] map1 = "path/to/map" map2 = "path/to/other/map"

seanyoung · 2023-06-13T13:03:44Z

src/bin/cli/mod.rs

+}
+
+#[derive(Args)]
+pub struct New {


Should this be called init, just like git init?

seanyoung · 2023-06-13T13:07:40Z

src/bin/cli/mod.rs

+}
+
+#[derive(Args)]
+pub struct New {


cargo allows cargo init and cargo new. git new does not exist

Signed-off-by: salaheldinsoliman <[email protected]>

…an/solang into config_from_toml

Signed-off-by: salaheldinsoliman <[email protected]>

xermicus · 2023-06-16T12:13:52Z

src/bin/cli/mod.rs

@@ -127,10 +261,22 @@ pub struct TargetArg {
    pub value_length: Option<u64>,
 }

+#[derive(Args, Deserialize, Debug, PartialEq)]
+pub struct CompileTargetArg {
+    #[arg(name = "TARGET", required_unless_present = "CONFFILE" ,long = "target", value_parser = ["solana", "substrate", "evm"], help = "Target to build for [possible values: solana, substrate]", num_args = 1, hide_possible_values = true)]


Is there a reason we need required_unless_present = "CONFFILE" here?

xermicus · 2023-06-16T12:16:13Z

src/bin/cli/mod.rs

-#[derive(Args)]
+#[derive(Args, Deserialize, Debug, PartialEq)]
+pub struct CompilePackage {
+    #[arg(name = "INPUT", help = "Solidity input files",value_parser = ValueParser::path_buf(), num_args = 1.., required_unless_present = "CONFFILE")]


Same as my comment above. Then, given a solang.toml file is present, we can just do solang compile (akin to how you can just cargo build in Rust projects). Which I'd find really neat

Good pointer, Thanks

Signed-off-by: salaheldinsoliman <[email protected]>

xermicus · 2023-06-19T06:21:20Z

docs/running.rst

+  solang new --target solana my_project
+
+Start a new Solang project with an example `flipper.sol <https://github.com/hyperledger/solang/blob/main/examples/solana/flipper.sol>`_ contract,
+next to an example configuration file.


Suggested change

next to an example configuration file.

and a default configuration file.

xermicus · 2023-06-19T06:25:23Z

docs/running.rst

+Starting a new project
+______________________________
+
+
+  solang new --target solana my_project
+
+Start a new Solang project with an example `flipper.sol <https://github.com/hyperledger/solang/blob/main/examples/solana/flipper.sol>`_ contract,
+next to an example configuration file.
+
+
+


This Paragraph needs to explain the user in more detail what exactly a "Solang project" is (that it creates a new directory with a default config file is just the product of this command, but we also need to explain the purpose of these Projects)

xermicus · 2023-06-19T06:29:06Z

examples/substrate/substrate_config.toml

+llvm-IR-optimization-level = "default"  # Set llvm optimizer level. Valid options are "none", "less", "default", "aggressive"
+


FIY I just merged #1365, could you include that here too?

xermicus · 2023-06-19T06:35:31Z

src/bin/solang.rs

+    match create_dir(&dir_path) {
+        Ok(_) => (),
+        Err(error) => {
+            eprintln!("couldn't create project directory, reason: {error}");
+            exit(1)
+        }
+    };


Suggested change

match create_dir(&dir_path) {

Ok(_) => (),

Err(error) => {

eprintln!("couldn't create project directory, reason: {error}");

exit(1)

}

};

if let Err(error) = create_dir(&dir_path) {

eprintln!("couldn't create project directory, reason: {error}");

exit(1);

}

xermicus · 2023-06-19T06:43:19Z

src/bin/cli/mod.rs

+
+impl PackageTrait for CompilePackage {
+    fn get_input(&self) -> &Vec<PathBuf> {
+        match &self.input {


Could use map_err instead of the match here

I think .expect() would look better

Panicking is not the same as std::process::exit(1). While technically the result is almost the same (I'm not sure whether we care about the different exit code). An unwrap or expect implies that this is a situation we do not anticipate for, and if it should ever happen regardless there's nothing to do about it. Here we can terminate in a more graceful way.

but a map_err would imply converting the Option<> to a Result<> beforehand. I think an if let Some() would suffice, what do you think?

Right, I didn't pay attention close enough, sorry. Yes you could return inside the if let and exit afterwards. Or leave it like it is, it's not terribly important

xermicus · 2023-06-19T06:45:01Z

src/bin/cli/mod.rs

@@ -218,10 +449,50 @@ pub(crate) fn target_arg(target_arg: &TargetArg) -> Target {
    target
 }

-pub fn imports_arg(package: &Package) -> FileResolver {
+pub trait PackageTrait {


This needs a comment explaining the trait and what it's used for

src/bin/solang.rs

LucasSte · 2023-06-19T12:09:03Z

docs/running.rst

+    name = "solana"  # Target name
+
+
+  Fields that are not explicitly present in the .toml file are defaulted. If any other argument is provided in the command line, for example, ``solang compile --config-file --target substrate``, the argument will be overridden.


Suggested change

Fields that are not explicitly present in the .toml file are defaulted. If any other argument is provided in the command line, for example, ``solang compile --config-file --target substrate``, the argument will be overridden.

Fields not explicitly present in the .toml acquire the compiler's default value. If any other argument is provided in the command line, for example, ``solang compile --config-file --target substrate``, the argument will be overridden.

LucasSte · 2023-06-19T12:11:12Z

docs/running.rst

+    name = "solana"  # Target name
+
+
+  Fields that are not explicitly present in the .toml file are defaulted. If any other argument is provided in the command line, for example, ``solang compile --config-file --target substrate``, the argument will be overridden.


Do I need to specify the .toml file I am referring to when I run solang compile --config-file? What happens when I have more than one .toml file in my repository?

If you are creating a .toml with a reserved name, that should be noted in the documentation.

The default one is solang.toml , but if a special name is used, it will be taken instead.

LucasSte · 2023-06-19T12:16:25Z

src/bin/solang_new_examples/solana/flipper.sol

+
+	/// Constructor that initializes the `bool` value to the given `init_value`.
+	@payer(payer)
+	constructor(address payer, bool initvalue) {


Suggested change

constructor(address payer, bool initvalue) {

constructor(bool initvalue) {

There is no need to pass the payer as an argument.

Signed-off-by: salaheldinsoliman <[email protected]>

seanyoung · 2023-06-21T08:40:55Z

examples/substrate/substrate_config.toml

+strength-reduce = true
+vector-to-slice = true
+common-subexpression-elimination = true
+llvm-IR-optimization-level = "default"  # Set llvm optimizer level. Valid options are "none", "less", "default", "aggressive"


This does not apply to llvm-IR only, also to target specific machine code. This should be called llvm-optimization-level

src/bin/solang_new_examples/solana/flipper.sol

src/bin/solang_new_examples/substrate/flipper.sol

xermicus

LGTM

docs/running.rst

Co-authored-by: Lucas Steuernagel <[email protected]> Signed-off-by: Cyrill Leutwiler <[email protected]>

salaheldinsoliman requested review from seanyoung, xermicus and LucasSte as code owners June 2, 2023 01:48

xermicus reviewed Jun 5, 2023

View reviewed changes

LucasSte requested changes Jun 5, 2023

View reviewed changes

salaheldinsoliman requested a review from xermicus June 7, 2023 06:53

fix vscode test

a3ef22e

Signed-off-by: salaheldinsoliman <[email protected]>

salaheldinsoliman force-pushed the config_from_toml branch from ee1978c to a3ef22e Compare June 9, 2023 19:39

xermicus reviewed Jun 9, 2023

View reviewed changes

src/bin/solang_new_examples/solana/flipper.sol Show resolved Hide resolved

src/bin/solang_new_examples/substrate/flipper.sol Show resolved Hide resolved

Merge branch 'hyperledger:main' into config_from_toml

c6f31c6

seanyoung reviewed Jun 13, 2023

View reviewed changes

salaheldinsoliman and others added 5 commits June 15, 2023 04:21

add docs/ improve import_maps

f2dec3c

Signed-off-by: salaheldinsoliman <[email protected]>

Merge branch 'hyperledger:main' into config_from_toml

7199c9b

add docs/ improve import_maps

f429d82

Signed-off-by: salaheldinsoliman <[email protected]>

Merge branch 'config_from_toml' of https://github.com/salaheldinsolim…

a05898b

…an/solang into config_from_toml

toml 0.7

01d5c11

Signed-off-by: salaheldinsoliman <[email protected]>

salaheldinsoliman requested a review from xermicus June 15, 2023 03:14

xermicus requested changes Jun 16, 2023

View reviewed changes

salaheldinsoliman added 2 commits June 19, 2023 03:35

merge main into config_from_toml

9faf0a1

Signed-off-by: salaheldinsoliman <[email protected]>

remove required_unless_needed

39ca8fd

Signed-off-by: salaheldinsoliman <[email protected]>

salaheldinsoliman requested a review from xermicus June 19, 2023 03:09

fix compilation without wasm_opt

b643486

Signed-off-by: salaheldinsoliman <[email protected]>

xermicus reviewed Jun 19, 2023

View reviewed changes

xermicus requested review from LucasSte, xermicus and seanyoung June 19, 2023 06:53

LucasSte reviewed Jun 19, 2023

View reviewed changes

salaheldinsoliman and others added 2 commits June 19, 2023 21:41

lints

d93c842

Signed-off-by: salaheldinsoliman <[email protected]>

Merge branch 'hyperledger:main' into config_from_toml

fa12307

seanyoung approved these changes Jun 21, 2023

View reviewed changes

xermicus approved these changes Jun 21, 2023

View reviewed changes

salaheldinsoliman requested a review from LucasSte June 21, 2023 18:44

LucasSte approved these changes Jun 22, 2023

View reviewed changes

docs/running.rst Outdated Show resolved Hide resolved

docs/running.rst Outdated Show resolved Hide resolved

Apply suggestions from code review

ed601fa

Co-authored-by: Lucas Steuernagel <[email protected]> Signed-off-by: Cyrill Leutwiler <[email protected]>

xermicus merged commit d772fe4 into hyperledger:main Jun 23, 2023
11 checks passed

	#[command(about = "Create a new project with a ready example")]
	#[command(about = "Create a new Solang project")]

	#[arg(name = "INPUT", help = "project name", num_args = 1, value_parser = ValueParser::os_string())]
	#[arg(name = "INPUT", help = "Name of the project", num_args = 1, value_parser = ValueParser::os_string())]

		pub struct Compile {
		#[arg(name = "CONFFILE", help = "Take arguments from configuration file", long = "configuration-file", exclusive = true, value_parser = ValueParser::os_string(), required_unless_present = "INPUT", num_args = 0..=1, default_missing_value = "Solang.toml")]

	eprintln!("error: must specifiy target name");
	eprintln!("error: compilation target not specified");

	next to an example configuration file.
	and a default configuration file.

		llvm-IR-optimization-level = "default" # Set llvm optimizer level. Valid options are "none", "less", "default", "aggressive"

		name = "solana" # Target name


		Fields that are not explicitly present in the .toml file are defaulted. If any other argument is provided in the command line, for example, ``solang compile --config-file --target substrate``, the argument will be overridden.

	Fields that are not explicitly present in the .toml file are defaulted. If any other argument is provided in the command line, for example, ``solang compile --config-file --target substrate``, the argument will be overridden.
	Fields not explicitly present in the .toml acquire the compiler's default value. If any other argument is provided in the command line, for example, ``solang compile --config-file --target substrate``, the argument will be overridden.

	constructor(address payer, bool initvalue) {
	constructor(bool initvalue) {

Read compiler configurations from toml #1347

Read compiler configurations from toml #1347

Conversation

salaheldinsoliman commented Jun 2, 2023

xermicus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xermicus Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salaheldinsoliman Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

xermicus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xermicus Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xermicus Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasSte Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xermicus left a comment

Choose a reason for hiding this comment

xermicus Jun 5, 2023 •

edited

Loading

salaheldinsoliman Jun 14, 2023 •

edited

Loading

xermicus Jun 19, 2023 •

edited

Loading

xermicus Jun 19, 2023 •

edited

Loading

LucasSte Jun 19, 2023 •

edited

Loading