hofstadter-io / hof

Framework that joins data models, schemas, code generation, and a task engine. Language and technology agnostic.
https://docs.hofstadter.io
Apache License 2.0
523 stars 37 forks source link

Nested module replace directives are ignored #75

Closed b4nst closed 1 year ago

b4nst commented 3 years ago

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.

verdverm commented 3 years ago

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.

b4nst commented 3 years ago

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
verdverm commented 3 years ago

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?

b4nst commented 3 years ago

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?

verdverm commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

b4nst commented 3 years ago

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?

verdverm commented 3 years ago

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?

b4nst commented 3 years ago

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

verdverm commented 3 years ago

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.

b4nst commented 3 years ago

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.

verdverm commented 3 years ago

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 commented 3 years ago

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

verdverm commented 1 year ago

closing this as all issues have been addressed