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

When --offline flag specified for forc build, attempt to use local git checkouts for git dependencies before failing #2366

Merged
merged 18 commits into from
Sep 26, 2022

Conversation

kayagokalp
Copy link
Member

closes #1787.

unblocks #2365.

@kayagokalp kayagokalp self-assigned this Jul 22, 2022
@kayagokalp kayagokalp requested review from mitchmindtree and a team July 22, 2022 20:17
@kayagokalp kayagokalp marked this pull request as ready for review July 22, 2022 20:17
@kayagokalp kayagokalp enabled auto-merge (squash) July 22, 2022 20:17
Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Nice one @kayagokalp! Just had a quick first review as I've been pushing back reviewing this for too long :)

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
@kayagokalp
Copy link
Member Author

Converting this into draft until we are done with fuel-client stuff as it has a higher priority.

@kayagokalp kayagokalp marked this pull request as draft August 8, 2022 22:24
auto-merge was automatically disabled August 8, 2022 22:24

Pull request was converted to draft

@kayagokalp
Copy link
Member Author

kayagokalp commented Sep 20, 2022

Quick update on my status :) @mitchmindtree, so as we discussed briefly yesterday, just storing .git in the checkouts could mean that different forc processes might get into a racing condition. Then I switched over to another approach, just storing the HEAD commit should significantly make the checkout smaller. Unfortunately only the CLI supports this, libgit does not support --depth. So git2-rs also cannot offer it libgit2/libgit2#3058. I am thinking of doing something like:

  1. Check if the user have git CLI installed and if the git repo is from github. If they have we can fetch it with --depth 1 and this should reduce the size. Like Improve performance of fetching git dependencies by rev rust-lang/cargo#10078.
  2. If we cant go with step 1, unfortunately we need to get the repo and the complete .git folder so that it can be queried later on.
    Does this sound reasonable? 👀

@kayagokalp
Copy link
Member Author

kayagokalp commented Sep 21, 2022

After a quick discussion over slack with @mitchmindtree, decided to go different path. I guess this is ready for a review 👍 Quick summary of what is happening here:

  1. In online mode, when forc fetches a dependency, it also creates a small indexing file .forc_index which holds the information about that repo (time of the HEAD commit, HEAD commit hash and type of the reference: branch, rev, tag). We are keeping this because we are omitting .git folder to save disk space and we need a way to import some information about the repo once we are in offline mode.
  2. Later on, when the user want's to build in offline mode, we search locally available checkouts. We do that by first filtering by the name of the package we are searching for and then we read the index file from each available checkout. If the git reference of the desired package is rev or tag. We search for the exact match. If the git reference of the desired package is branch we look for the best match. Best match here means, the most recent commit we have for the desired branch.

Since .forc_index is generated deterministically (for the same repo/reference we are generating the exact same .forc_index file), multiple instances of forc can work together doing the same checkout. Since the file content in the end will be intact, there won't be a problem later on when we got into offline mode. Similarly in the offline mode, multiple instances of forc can work with the same checkout as they will only read the indexing file.

Only possible problem might be with a forc process fetching a repo that already exists in online mode , while another forc process trying to find it in the offline mode. Since before the checkout is completed we check if the target dir exists. If that is the case we are deleting it. So the time between the deletion of the target folder and the checkout, the searching forc process won't be able to find the repo it is looking for. I believe this can also happen before this PR too, because the checkout is done after everything is removed and there is a small time-window where we don't have the repo.

I tried to run 16 concurrent build operation 10 times and couldn't catch something like this. I am not sure if I am missing something but, if this seems possible and significant enough we can introduce something like a .forc_ready file. If a .forc_read file is not present, searching could wait until it is there (with a time limit). I believe i saw something similar in cargo somewhere but couldn't find it now 🤔

My Testing Procedure:

  1. Create a new project
  2. Build the project with implicit std (default) in online mode
  3. Add std as an explicit dependency and point to master branch
  4. Build the same project in online mode.

After these steps you will have both master and latest tag checkouts in your local.

  1. Remove the explicit std dependency declaration from the toml file
  2. Build the project with --offline flag
  3. Add back the std explicit dependency which points to master branch
  4. Build the project with --offline flag

@kayagokalp kayagokalp marked this pull request as ready for review September 21, 2022 13:09
@kayagokalp kayagokalp requested review from a team and mitchmindtree September 21, 2022 13:09
Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Awesome stuff @kayagokalp, this is looking great! Just a few nits then happy to approve 👍

I believe this can also happen before this PR too, because the checkout is done after everything is removed and there is a small time-window where we don't have the repo.

Yeah, that's correct! See #2603 which managed to trigger this with a very specific setup on CI. So far it's remained rare enough that this hasn't been a big priority just yet.

I am not sure if I am missing something but, if this seems possible and significant enough we can introduce something like a .forc_ready file. If a .forc_read file is not present, searching could wait until it is there (with a time limit). I believe i saw something similar in cargo somewhere but couldn't find it now 🤔

I think the long-term solution here might be something along the lines of #919. Edit: to clarify, we can leave that for a future PR!

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

/// Search local checkouts directory and apply the given function. This is used for iterating over
/// possible options of a given package.
fn with_search_checkouts<F>(checkouts_dir: PathBuf, package_name: &str, f: &mut F) -> Result<()>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn with_search_checkouts<F>(checkouts_dir: PathBuf, package_name: &str, f: &mut F) -> Result<()>
fn with_search_checkouts<F>(checkouts_dir: PathBuf, package_name: &str, f: F) -> Result<()>

We can take the function directly, no need to take by &mut as &mut F also implements FnMut :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm it seems like if I do this I am getting a compiler error at f(index, repo_dir_path) line complaining about not being able to capture it mutably. Once I remove the unnecessary &mut statements in front of the closures I pass to this function and change function signature to fn with_search_checkouts<F>(checkouts_dir: PathBuf, package_name: &str, mut f: F) -> Result<()> it works. I might be missing something but to get it compiling I had to go like the doc example

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved
@kayagokalp kayagokalp requested review from mitchmindtree and a team September 22, 2022 11:00
@kayagokalp kayagokalp enabled auto-merge (squash) September 22, 2022 11:00
@kayagokalp kayagokalp requested review from sezna and a team September 22, 2022 11:03
@kayagokalp kayagokalp merged commit 3176c1f into master Sep 26, 2022
@kayagokalp kayagokalp deleted the kayagokalp/1787 branch September 26, 2022 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants