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

Implicitly add std dependency if unspecified #1137

Merged
merged 10 commits into from
Apr 6, 2022

Conversation

mitchmindtree
Copy link
Contributor

@mitchmindtree mitchmindtree commented Apr 4, 2022

While loading a Manifest with the from_file constructor, we check for dependencies declared with either std or core packages.

If neither core or std exist, we add their associated git dependencies pointing to this sway repo at the tag equal to forc's current semver version. This way, the implicit core and std deps are pinned alongside the version of forc that fetches them, guaranteeing that the manifests are compatible and that the core and std versions are compatible with eachother.

If both core and std are supplied by the user, or if only one of either core or std are provided by the user, then we don't implicitly include either. This is because in the case that only either core or std are provided, we cannot guarantee that an implicitly included core or std would be compatible with the user-specified one.

Closes #330

Sway repo examples/tests

It's best we leave the examples and test suite with the explicit path dependencies in order to ensure that they remain tested against the current state of forc rather than the latest tagged commit. This will also avoid running into CI issues during the PRs where forc's version is updated but the tag hasn't yet been pushed.

This also adds a core dependency declaration to all tests that otherwise contained an empty [dependencies] table. By overriding either the core or std dependency manually, we disable the implicit inclusion of the std dependency.

TODO

  • Pass forc semver version through to Manifest::from_file constructor in order to set tag for core and std git dependencies.
  • Ensure build plan validation works with implicit core/std.

@adlerjohn
Copy link
Contributor

adlerjohn commented Apr 4, 2022

Rather an implicitly including both core and std, why not just std? Basically 100% of cases users will only include std (ref #973). The only people using core directly should be standard library developers, and that will require local paths anyways.

@mitchmindtree
Copy link
Contributor Author

why not just std?

Good point! I was blindly following Rust in this case where both core and std are included, but I think Rust does this to support the #![no_std] use-case which isn't really relevant for Sway.

@mitchmindtree mitchmindtree changed the title Implicitly add core and std dependencies if unspecified Implicitly add std dependency if unspecified Apr 4, 2022
@mitchmindtree mitchmindtree force-pushed the mitchmindtree/implicit-core-std branch from 4276868 to ce63830 Compare April 4, 2022 06:04
@mitchmindtree mitchmindtree marked this pull request as ready for review April 4, 2022 06:16
JoshuaBatty
JoshuaBatty previously approved these changes Apr 5, 2022
Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

utACK.

TODO: Pass `forc` semver version through to `Manifest` `from_file`
constructor in order to set `tag` for `core` and `std` git dependencies.
This is necessary in order to pin the version of the implicit `std`
dependency to the version of `forc`.
By overriding either the `core` or `std` dependency manually, we disable
the implicit inclusion of the `std` dependency which may have caused CI
issues in the future when `forc`'s version is updated before the git tag
for the new version is made available.
@mitchmindtree
Copy link
Contributor Author

Just rebased onto master after the merge of #1105 cc @JoshuaBatty

@JoshuaBatty JoshuaBatty self-requested a review April 5, 2022 06:17
Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

LGTM, but would like @sezna to confirm that there's nothing nuanced we missed

Copy link
Contributor

@sezna sezna left a comment

Choose a reason for hiding this comment

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

Seems reasonable. Do you want to do the thing we mentioned regarding omitting std as a Forc.toml flag as a part of this PR, or as a follow-up? If the latter, would you mind making an issue to track it?

@adlerjohn
Copy link
Contributor

Filed #1150 to track removing std from default Forc manifest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forc lib: core Core library lib: std Standard library
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Implicitly include std and core when undeclared in the Forc.toml
4 participants