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

Change forc's git dependencies to not rely on the GitHub REST API. Use git tags to pin to versions instead. #829

Closed
mitchmindtree opened this issue Feb 23, 2022 · 2 comments · Fixed by #825
Assignees

Comments

@mitchmindtree
Copy link
Contributor

mitchmindtree commented Feb 23, 2022

Currently forc doesn't really support git dependencies generally, but rather calls into the GitHub REST API.

As a part of #825, I'm aiming to switch us to a more genuine/generalised git support like cargo has, however doing so will mean changing the way we pin to specific versions for git dependencies.

The current way that std and core git dependencies are pinned relies on using the GitHub-specific REST API to fetch the pinned "release" tarball. Under cargo git dependencies (and git itself), the term "version" doesn't really mean anything, as git dependencies are normally pinned to a particular commit either directly (rev), under a tag or under a given git branch. In order to emulate the current behaviour where we pin std and core to a particular version, I'd recommend we instead add a git tag to the commit that publishes the new version and pin the dependency using the version tag (e.g. tag = "v0.1.0").

@mitchmindtree mitchmindtree self-assigned this Feb 23, 2022
mitchmindtree added a commit that referenced this issue Feb 23, 2022
This is in preparation for addressing #799 and makes landing #825 a
little easier for the reasons described in #829.

We might wish to consider keeping these dependencies pointed at the
master branch, however this would be made easier by #830.
mitchmindtree added a commit that referenced this issue Feb 23, 2022
This is in preparation for addressing #799 and makes landing #825 a
little easier for the reasons described in #829.

We might wish to consider keeping these dependencies pointed at the
master branch, however this would be made easier by #830.
mitchmindtree added a commit that referenced this issue Feb 23, 2022
Addresses #829.

Also allows for putting off #831 while we discuss #830.
mitchmindtree added a commit that referenced this issue Feb 23, 2022
See #829.

This also temporarily disables `forc update` and the related
`forc_dep_check` module pending more general `git` support and
committing updates to a `Forc.lock` file.
@adlerjohn adlerjohn moved this to Todo in Fuel Network Feb 24, 2022
@adlerjohn
Copy link
Contributor

we instead add a git tag to the commit that publishes the new version and pin the dependency using the version tag

I don't think we need to separately do that; cutting a release on GitHub automatically creates a tag.

@mitchmindtree
Copy link
Contributor Author

I did notice that we already had the necessary tags late last night! I've added a commit to the WIP build plan PR here that makes use of the new tag field (replacing the version field) in the tests.

mitchmindtree added a commit that referenced this issue Feb 24, 2022
Addresses #829.

Also allows for putting off #831 while we discuss #830.
mitchmindtree added a commit that referenced this issue Feb 24, 2022
See #829.

This also temporarily disables `forc update` and the related
`forc_dep_check` module pending more general `git` support and
committing updates to a `Forc.lock` file.
mitchmindtree added a commit that referenced this issue Feb 28, 2022
Addresses #829.

Also allows for putting off #831 while we discuss #830.
mitchmindtree added a commit that referenced this issue Feb 28, 2022
See #829.

This also temporarily disables `forc update` and the related
`forc_dep_check` module pending more general `git` support and
committing updates to a `Forc.lock` file.
mitchmindtree added a commit that referenced this issue Feb 28, 2022
See #829.

This also temporarily disables `forc update` and the related
`forc_dep_check` module pending more general `git` support and
committing updates to a `Forc.lock` file.
mitchmindtree added a commit that referenced this issue Feb 28, 2022
Addresses #829.

Also allows for putting off #831 while we discuss #830.
mitchmindtree added a commit that referenced this issue Feb 28, 2022
Addresses #829.

Also allows for putting off #831 while we discuss #830.
mitchmindtree added a commit that referenced this issue Mar 2, 2022
See #829.

This also temporarily disables `forc update` and the related
`forc_dep_check` module pending more general `git` support and
committing updates to a `Forc.lock` file.
mitchmindtree added a commit that referenced this issue Mar 2, 2022
Addresses #829.

Also allows for putting off #831 while we discuss #830.
mitchmindtree added a commit that referenced this issue Mar 2, 2022
Addresses #829.

Also allows for putting off #831 while we discuss #830.
mitchmindtree added a commit that referenced this issue Mar 3, 2022
See #829.

This also temporarily disables `forc update` and the related
`forc_dep_check` module pending more general `git` support and
committing updates to a `Forc.lock` file.
mitchmindtree added a commit that referenced this issue Mar 3, 2022
Addresses #829.

Also allows for putting off #831 while we discuss #830.
mitchmindtree added a commit that referenced this issue Mar 4, 2022
…rt. Add `Forc.lock` for more deterministic builds. (#825)

* Construct `pkg_graph` at beginning of `forc build`

This is a start at separating the construction of the dependency graph
and fetching of dependency source from the compilation process. This is
done under a new `fetch_pkgs` function where, given the manifest of the
package we're compiling, it produces a graph where each node represents
a package at a known, pinned version/commit along with a `Path` to a
local copy of their source (under `.forc/git/checkouts` for git deps).

The `pkg_graph` also contains our root-level package, allowing us to
treat all packages in the compilation process as equal, avoiding special
case code for the top-level program.

The compilation order of packages is determined by performing a toposort
of the `pkg_graph`. By compiling packages in this order, each node is
always guaranteed to have access to the compiled artifacts of each of
its dependencies.

This PR also introduces the `git2` crate for fetching and working with
git dependencies. This should allow us to remove the current
github-specific dependency handling in favour of more general git
support.

---

I think the next step in this PR will be replacing the existing
`dependency_graph` mutable hash map reference with an immutable
reference to the newly constructed `pkg_graph`. It seems this may also
involve populating the `Namespace` prior to the rest of compilation too?
I'm still familiarising myself with `sway-core`, so will need to dive a
bit deeper to be sure.

* Replace old dependency compilation with new `pkg_graph` approach

This removes the old dependency-specific compilation logic in favour of
flattening the graph into a list of compilation steps and using the same
`compile_pkg` function to compile all packages, including at the
top-level package.

* Move `pkg`-specific code into a new `pkg` submodule

* Address cargo fmt and clippy nits

* Remove implied pkg from idents in pkg submodule

* Add support for specifying git dependency via tag

* Fix dependency path handling to be relative to parent

* Remove github-specific logic in favour of new git pkg handling

See #829.

This also temporarily disables `forc update` and the related
`forc_dep_check` module pending more general `git` support and
committing updates to a `Forc.lock` file.

* Improve support for determinism and cachability with `Forc.lock`

This adds a `Forc.lock` file that for the most part mirrors the
`Cargo.lock` file.

If a `Forc.lock` doesn't exist, a `Lock` structure is created from the
package graph and serialised to TOML before being written to the
`Forc.lock` file path.

If a `Forc.lock` *does* exist, we construct the build graph from the
`Lock` structure, re-using the previously fetched source for the pinned
dependencies.

TODO:

- Make `forc update` update the lock file.
- Add support for updating individual packages.

* Move `BuildPlan` under `pkg` module in anticipation for `forc update`

* Update `forc update` for lock. Print removed and added deps.

A new `lock::Diff` type is added to collect added/removed packages.

* Address doc comment nits

* Validate `Lock` in accordance with `Manifest`

This ensures that if any `Forc.toml` dependencies are added, removed or
modified, the change is detected during `forc build` and the `Forc.lock`
file is updated accordingly.

* Improve formatting of `Forc.lock` related stdout

* Update sway-core petgraph version so that it matches forc

* Update test and examples to pin via git tag rather than version field

Addresses #829.

Also allows for putting off #831 while we discuss #830.

* Update some E2E tests due to unpinned `core` dep in `std`

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.

* Only fetch pkgs that are missing, rather than whole pkg graph

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.

* Update forc git dep from 0.13 to 0.14

* Fix forc Cargo.toml dependency order

* Update Cargo.lock for the addition of git2, petgraph

* Add Forc.lock files for sway examples

* Ensure `Namespace` only contains a package's dependencies

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.

* Pin tests by adding their `Forc.lock` files for reproducibility

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
```

* Temporarily warn about `version` field in dependencies

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.

* Address nits uncaught in anyhow PR review

* Fix formatting following rebase onto forc anyhow PR
Repository owner moved this from Todo to Done in Fuel Network Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
2 participants