hof icon indicating copy to clipboard operation
hof copied to clipboard

Nested module replace directives are ignored

Open b4nst opened this issue 2 years ago • 14 comments

As written here

https://github.com/hofstadter-io/hof/blob/ac766d2494f3968a0e2836a52fd206ae278d4563/lib/mod/modder/modder_vendor.go#L153

module replace directive are ignored for nested dependencies. But since the original directive is put in cue.sums, it fails when having nested dependencies.

One simple way to reproduce it is by including a dependency that depends on another lib using a replace statement.

If I do not ignore the replace directive:

err = m.LoadMetaFiles(mdr.ModFile, mdr.SumFile, mdr.MappingFile, false) 

it does work, but I suspect there is a reason behind this ignore that I missed. Another way to fix it - I guess - would be to use the replace directive inside the cue.sums file.

b4nst avatar Sep 21 '21 15:09 b4nst

Nested replaces are intentionally ignored, as this is what Go does.

You'll have to replicate the replace if you want to use a dependency with replaces. The problem is more evident when you consider two deps with different replacements on a mutual dep. In general, sum files are not considered when using MVS, just the mod file.

verdverm avatar Sep 21 '21 19:09 verdverm

Gotcha, thank you! That being said, even my local replace are ignored for nested dependencies. Here one example of what I'm trying to do:

require (
    gitlab.com/nested/crap/foo v0.6.0
    gitlab.com/nested/crap/bar v0.3.1
)

replace gitlab.com/nested/crap/foo => gitlab.com/nested/crap/foo.git v0.6.0
replace gitlab.com/nested/crap/bar => gitlab.com/nested/crap/bar.git v0.3.1

bar depends on foo v0.3.0. But

> hof mod vendor cue
fetch:  cue  gitlab.com/nested/crap/foo v0.3.0 true
gitlab git fallback
While fetching from gitlab
repository not found

The repository is not found because the replace did not happen (no .git). I would expect

fetch:  cue  gitlab.com/nested/crap/foo.git v0.3.0 true

or even

fetch:  cue  gitlab.com/nested/crap/foo.git v0.6.0 true

b4nst avatar Sep 22 '21 14:09 b4nst

hmm, that is tricky. Is it the case that the bar mod file does not have a .git on the end of the foo dep line?

Generally, if bar is processed before foo, it still needs to fetch its dependencies to see if there are more dependencies (process deps' mod file). So you may fetch multiple versions of the same repository.

I'd have to think through if a shortcut to skip fetching would break things. I suspect it would.

Maybe we can add a rule such that gitlab deps have a .git appended when not found?

verdverm avatar Sep 22 '21 15:09 verdverm

We could append a .git but it sounds more like a hacky way to fix that. Shouldn't the local replace be used whenever a sub dependency depends on a module that has a local replace directive?

b4nst avatar Sep 22 '21 16:09 b4nst

I think the problem is that other, transitive dependencies may be discovered through MVS and so fetching to process the mod file is still required, despite the current module version not being used.

Will have to think on it. Creating runnable reproducers will likely help.

verdverm avatar Sep 22 '21 18:09 verdverm

taking a step back, are the replaces solely for the .git suffix? Is that the root of the problem?

It looks as though Go supports them on the require line for gitlab, but not github

verdverm avatar Sep 23 '21 03:09 verdverm

The Go MVS algorithm is a BFS, so it is entirely possible to discover an earlier version prior to a later version.

https://github.com/golang/go/blob/master/src/cmd/go/internal/mvs/mvs.go#L111

verdverm avatar Sep 23 '21 04:09 verdverm

Yes the replaces are solely to avoid .git extension in every imports. Sorry I'm quite new to the MVS algorithm. Since it's a BFS, I guess we start by reading the root cue.mods. Would it be dangerous to use the root replace directives but still keeping the version stated in the sub-module mod file?

b4nst avatar Sep 23 '21 16:09 b4nst

replaces aren't important for this problem, it could be reproduced without them, because of BFS

The real problem is gitlab being a special case. I'd try the "add .git if not found on gitlab". Is the .git not supported in hof's required line?

verdverm avatar Sep 23 '21 17:09 verdverm

I think .git is supported in hof's required line. I just didn't want to write it everywhere. I'm gonna try multiple solution to check whether they're working or not. If we "add .git if not found on gitlab" we're gonna need to recurse it until host/user/project in case of mono repo. Not the best in terms of performances, but could work

b4nst avatar Sep 29 '21 09:09 b4nst

Technically, Go tries to fetch down the path recursively until it finds a go.mod response. Thing is the code hosting providers have enabled a special query param just for Go.

Maybe just not support monorepos? I think that applies to all hosts

The easiest thing would be to just append .git, I think it should be a one line change. That would mean you don't have to write it everywhere.

Doing the recursive fetching is certainly more complicated and will require more work than I have time to make priority for.

verdverm avatar Sep 29 '21 15:09 verdverm

Doing the recursive fetching is certainly more complicated and will require more work than I have time to make priority for.

Yup that's understandable.

I agree adding .git for GitLab host would be a quick fix. I'll try to open a PR this weekend. I do not have any GitLab hosted monorepo right now, so I don't need the complex recursing thing. If at some point I face the issue, I'll take the time to propose a proper PR tackling that.

b4nst avatar Sep 30 '21 10:09 b4nst

sgtm, I think this one may only require a small addition to the gitlab.Fetch function. That seems the best place to catch it without having to worry about the memoization going on.

verdverm avatar Sep 30 '21 14:09 verdverm

Of interest: https://golang.org/ref/mod#vcs-find and https://golang.org/ref/mod#resolve-pkg-mod

verdverm avatar Oct 04 '21 22:10 verdverm

closing this as all issues have been addressed

verdverm avatar Mar 09 '23 03:03 verdverm