Add forc BuildPlan. Change GitHub dependencies to general git support. Add Forc.lock for more deterministic builds.
#825
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mitchmindtree
added a commit
that referenced
this pull request
Feb 22, 2022
While working on #825 I noticed that that the `dependency_graph` is unused throughout `sway-core`. I think the reason why cargo doesn't emit any warnings about this is that at one point, it's stored in the public `TypeCheckArgument` struct, so cargo believes that it is intentionally being re-exported, however in reality we don't use it anywhere else in the fuel/sway/forc ecosystem.
mitchmindtree
added a commit
that referenced
this pull request
Feb 22, 2022
While working on #825 I noticed that that the `dependency_graph` is unused throughout `sway-core`. I think the reason why cargo doesn't emit any warnings about this is that at one point, it's stored in the public `TypeCheckArgument` struct, so cargo believes that it is intentionally being re-exported, however in reality we don't use it anywhere else in the fuel/sway/forc ecosystem.
8bef3c2 to
95a68b5
Compare
mitchmindtree
added a commit
that referenced
this pull request
Feb 22, 2022
* Remove the unused `&mut dependency_graph` throughout `sway-core` While working on #825 I noticed that that the `dependency_graph` is unused throughout `sway-core`. I think the reason why cargo doesn't emit any warnings about this is that at one point, it's stored in the public `TypeCheckArgument` struct, so cargo believes that it is intentionally being re-exported, however in reality we don't use it anywhere else in the fuel/sway/forc ecosystem. * Remove trailing commas uncaught by rustfmt * Remove unused `dependency_graph` from sway-core selector_debug feature
13f2cf4 to
fecb92e
Compare
8a881e5 to
106e156
Compare
mitchmindtree
commented
Feb 24, 2022
05be535 to
806fb5c
Compare
pkg_graph at beginning of forc buildBuildPlan. Change GitHub dependencies to general git support. Add Forc.lock for more deterministic builds.
8bd7ebd to
d896d0c
Compare
None of the `std` releases so far pin `core` to a version. As a result, it pulls the `core` master branch by default, despite the top-level project also dependending on `core` at a specific version. Most E2E tests seemt to work regardless, but this subset requires updating. Following the lock file work landing, we should update versioned `std` releases to refer to versioned `core` via git tag.
Previously if any of the pkgs that exist in `Forc.lock` were not already available in `~/.forc/git/checkouts`, we would update the entire lock file and re-fetch all dependencies. Now, if the local copy of a pkg is missing, we just fetch that individual pkg and continue. This commit also updates the `fetch_pkgs` code to re-use pre-fetched git commits. Previously, all packages would be re-fetched when the `Forc.lock` file was updated. Now, we only fetch if the path for that commit doesn't already exist.
Previously one namespace was used to collect *all* items. This meant that when passed to the package that was being compiled, the namespace contained not only the names for its dependencies, but sometimes other unrelated names that happened to be earlier in the compilation order. This commit fixes this issue, creating a fresh namespace purely for each package in order to collect only its dependencies. This fixes some shadowing errors that were appearing in the E2E tests.
As a follow-up, we should update all of the tests to depend on `master` and then update the lock files. This will save us from having to change versions tags to pin all the time. Note: You can clear all test lock files with ``` rm ./test/src/e2e_vm_tests/test_programs/*/Forc.lock ```
We no longer use the GitHub REST API and in turn no longer map the `version` field to GitHub "release"s. Instead, we now support git more generally. Warn about appearances of the `version` field in dependencies and suggest using `branch` or `tag` as an alternative.
881d638 to
edd39aa
Compare
|
OK, just rebased onto master again now that #860 has landed. |
JoshuaBatty
approved these changes
Mar 3, 2022
sezna
approved these changes
Mar 4, 2022
mitchmindtree
added a commit
that referenced
this pull request
Mar 4, 2022
mitchmindtree
added a commit
that referenced
this pull request
Mar 4, 2022
This was referenced Mar 4, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
OK, this should be ready for review!
Apologies for the massive PR! Originally I only intended to refactor
forc_buildso that the process of fetching dependencies and determining the package graph could be distinct from the process of compilation (see #798), however upon beginning I figured it made sense to also address #829 (remove GitHub-only git repo requirement) as the process of building the package graph is tightly coupled with the fetching of those packages. This lead to addressing #746 (adding proper namespace for forc's git checkouts) as the existing directory structure didn't make sense for more generalised git support. In order to allow for safely caching/re-using pre-checked-out builds, I realised I needed the ability to pin specific commits, so I also ended up addressing #799 as a result. In hindsight, I would have been better off splitting off #829 into a future PR 😅 but it's done now.Edit: Keep in mind about ~2K of the additions are just the new
Forc.lockfiles.BuildPlan(#798)The new
BuildPlantype is an internal detail that represents a forc project's entire build plan. This includes the package graph, a map from all packages to a local copy of their src, and the compilation order determined by performing a toposort of the package graph.The package
GraphThe package graph represents the dependencies between all packages within the project. Each node represents a pinned package. The edge a -> b implies that a depends on b.
Pinning,
Lock&Forc.lock(#799)Pinning a dependency means fetching its source, determining the latest commit for the given branch (or the latest version that adheres to semver in the case of upcoming registry support) and then ensuring that we can deterministically make use of this same commit or version within future builds.
To achieve this, this PR introduces the
Forc.lockfile (directly inspired byCargo.lock) along with aLockstructured representation. TheLocktype is a toml-serialization-friendly version of the package graph that, when serialized to TOML and written toForc.lock, acts as a human-readable, git-diff-friendly representation of the project's graph of pinned dependencies.By using a lock file to represent pinned dependencies, we can allow for better reproducibility in forc projects without requiring that users write the exact pinned version or commit hashes manually. Instead, users can run
forc updateto update the commit (or version) for each pinned dependency automatically.forc buildwill automatically create a newForc.lockfile if one didn't exist, or if the existing file doesn't match the current set of dependencies in theForc.tomlmanifest. Bothforc updateandforc buildwill print a diff of theLockwhen updating theForc.lockfile. This involves first printing removed packages in red, and added packages in green.GitHub -> git (#829, #746)
This PR removes all the GitHub-specific code under handling of git dependencies in favour of more general git support using the
git2crate.Previously, forc used the
versionfield for git dependencies to imply a GitHub "release". The GitHub REST API was used to fetch this release. This PR removes this behaviour, asversiondoesn't really have any particular meaning under git itself. Instead, this adds support for pinned git branches and git tags.Unfortunately,
git2does depend on openssl cc @adlerjohn. It looks like there's some recent interest in landing a rustls+hyper backend discussed here along with a WIP that could potentially resolve this dependency in the future.E2E tests & examples
For now, examples and E2E tests have been updated to use git tags instead. However, following this PR we should consider updating all examples and E2E tests to point to master and instead commit the Forc.lock file to pin each project's versions.
One issue that updating the tests and examples has revealed is that currently, the
stddependency doesn't actually specify a pinned version (or tag) forcore. As a result, all E2E tests and examples that use some version or tag instead of themasterbranch for thecoredependency end up with two versions ofcorein theirForc.lockfile: 1. thecoremaster branch as defaulted to in thestdlibrary's dependencies and 2. thecoreversion listed under the top-level project's dependencies. Fortunately this doesn't seem to cause any issues in most examples. Those few that did have issues have been updated.Closes #746.
Closes #798.
Closes #799.
Closes #829.
TODO
Namespacepopulation is affected by this change.if_elseif_elsetest example isn't building. Edit: This is due to theversionfield ingitdependencies no longer having meaning. See Change forc'sgitdependencies to not rely on the GitHub REST API. Use git tags to pin to versions instead. #829 and Update E2E tests to point to master branch ofcore,std#831.githandling approach. See Change forc'sgitdependencies to not rely on the GitHub REST API. Use git tags to pin to versions instead. #829.Forc.lockfile inspired byCargo.lockAllow for "pinning" dependencies to better support reproducible builds, i.e.Forc.lock#799. Always attempt to load the graph from the lock file first. Onforc update, update the lock file. The lock file feature must be implemented in order to allow for re-using pre-fetched dependencies - otherwise we need to re-fetch dependencies on every build which hugely increases build times, especially for E2E tests.forc updatesimply update theForc.lockfile.forc's[dependencies].Forc.lockworks even if~/.forc/git/checkoutsdoesn’t exist.Forc.lockfiles for all examples and E2E tests for better reproducibility and faster compilation by avoiding fetching the same deps for every package.Follow-up
forc build --plan-onlyflag or similar that outputs thepkg_graphvia JSON or DOT. For now, downstream can always reconstruct the graph from theForc.lock, but it's not particularly convenient.forc update --package <name>.