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

Use package name instead of lib name, default to "rlib" #929

Merged
merged 13 commits into from
Jan 30, 2023

Conversation

ascjones
Copy link
Collaborator

Closes #927.

I have a vague memory of some reasoning that the package name and lib name were separate. However I cannot remember or imagine what it might be, so LFG.

@ascjones
Copy link
Collaborator Author

CI errors should be fixed in #930

@athei
Copy link
Contributor

athei commented Jan 26, 2023

I think rlib is the default for libs anyways. Do we even need crate-type in the manifest then?

@HCastano
Copy link
Contributor

I think rlib is the default for libs anyways. Do we even need crate-type in the manifest then?

From here it doesn't necessarily say that it's the default. It just says that the compiler will choose the best option

@@ -11,11 +11,12 @@ scale = { package = "parity-scale-codec", version = "3", default-features = fals
scale-info = { version = "2.3", default-features = false, features = ["derive"], optional = true }

[lib]
name = "{{name}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this goes back to at least 2019 🤷

@ascjones
Copy link
Collaborator Author

I think rlib is the default for libs anyways. Do we even need crate-type in the manifest then?

I've tested this with the delegator ink! example, removing the crate-type section from the adder manifest, and the it compiles fine with the same resulting contract size.

@athei
Copy link
Contributor

athei commented Jan 27, 2023

I think rlib is the default for libs anyways. Do we even need crate-type in the manifest then?

From here it doesn't necessarily say that it's the default. It just says that the compiler will choose the best option

Yes. But the default should always allow depending on it from another crate. We actively prevented this by setting it to only cdylib before this PR. I just tested without any crate type: I can depend on a contract.

I've tested this with the delegator ink! example, removing the crate-type section from the adder manifest, and the it compiles fine with the same resulting contract size.

This was expected because we set it to cdylib during build.

crates/build/src/crate_metadata.rs Outdated Show resolved Hide resolved
crates/build/src/lib.rs Outdated Show resolved Hide resolved
@ascjones ascjones merged commit da24337 into master Jan 30, 2023
@ascjones ascjones deleted the aj/template-changes branch January 30, 2023 20:04
@HCastano
Copy link
Contributor

@ascjones can you add a CHANGELOG entry since this changes the default behaviour of contract dependencies?

@ascjones ascjones mentioned this pull request Feb 14, 2023
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.

cargo-contract new should not emit name for the [lib] section
3 participants