full module path for api

[deitch]

This PR removes any relative replace paths for api/go, e.g. this one:

replace github.com/lf-edge/eve/api/go => ../../api/go

This has some benefits and a drawbacks.

On the benefits side, it eliminates more replace clauses, which are unfriendly to many things, including using things in libraries and using go install. More importantly., it eliminates the last vestiges of relative paths in pillar and a few other places, so the compiled binaries include explicit modules.

The drawback is that it requires an additional step to make api changes. @eriknordmark pointed this out with the following example.

Current process to make api changes:

1. edit proto files
2. make proto - builds api/go, api/python, etc. from the updated proto files
   3.make proto-vendor - updates pkg/pillar/vendor with the latest version from the relative path
3. use new API fields in *.go files
4. make pkg/foo - e.g. pkg/pillar
5. Open a single PR that includes both API changes and downstream pkg changes

If this PR goes through, we have something a little bit longer

1. edit proto files
2. make proto - builds api/go, api/python, etc. from the updated proto files
3. Open a PR for just the API changes, get it approved, merge it in
4. In the downstream dependency, e.g. pillar, do go get github.com/lf-edge/eve/api/go@master && go mod tidy && go mod vendor - these replace make porto-vendor, but we easily could just have make proto-vendor do all of this for us.
5. use new API fields in *.go files
6. make pkg/foo - e.g. pkg/pillar
7. Open a PR that includes only downstream pkg changes

The key here is that api/go starts being treated as a standalone upstream dependency of pillar and other packages, like any other; controllers (e.g. Zedcloud or Adam) have to function that way already.

The reverse argument is that eve's API is, well, for eve. Maybe it should be tied tightly together with eve itself.

It is my opinion that this does not go far enough, that api should be its own repo, with a smaller and lighter CI, that need only test for the ability to compile the protobufs , and some tests for backwards compatibility.

The dev process might still include temporary replace lines in go.mod, just like any other upstream dependency which is not in the same repository.

I am opening this PR to prompt the discussion.

[deitch]

I do have a third alternative, which would eliminate replace dependencies but keep it as a single PR for updates. Also has some pros and cons.

Have make copy api/go dependencies into downstream, e.g. pkg/pillar. We already vendor it, so instead of vendoring, we could just move it in:

1. edit proto files
2. make proto - builds api/go, api/python, etc. from the updated proto files
3. make proto-vendor - copies all api/go into pkg/pillar/api/go/; it no longer would be in go.mod or vendor/
4. use new API fields in *.go files
5. make pkg/foo - e.g. pkg/pillar
6. Open a single PR that includes both API changes and downstream pkg changes

Is this better? I still lean towards separate PRs, maybe even separate repos, as these are separate things. But I can see the "these are close together" arguments (although an independent API repo would encourage other implementations). If we want both of those, we can go down that path.

[eriknordmark]

@deitch Without the replace directives, if we still have make proto-vendor, shouldn't it work the same as today?
Shouldn't the build of pillar, newlog and edgeview will pick up the pb.go files from their vendor directory even without the replace?
Can you try in your branch by removing a field in a .proto field and do make proto proto-vendor pkg/pillar and see if it fails due to the missing field?

If this works then I have one more RFE which is to have make proto-vendor handle not only pillar but also newlog and edgeview.

[deitch]

If this works then I have one more RFE which is to have make proto-vendor handle not only pillar but also newlog and edgeview.

Sounds fine, but let's do that in two PRs? One that changes current functionality, another that expands that functionality.

[deitch]

Without the replace directives, if we still have make proto-vendor, shouldn't it work the same as today?
Shouldn't the build of pillar, newlog and edgeview will pick up the pb.go files from their vendor directory even without the replace?

You mean the structure in this PR? No, it wouldn't, because we just do cd pkg/pillar; go mod vendor, which just reads whatever is in go.mod and vendors it. Without the replace, it is just going to pull whatever version is in go.mod from the Internet.

If we take the alternative, wherein pillar doesn't include api/go in go.mod at all, but instead we use some Make magic to copy them in as source, then, sure, it would work the same. We wouldn't need it in vendor/, because it is local. But now you have duplicate copies.

Well, we could use go generate. Then. rather than copying it via make proto-vendor, we use go generate to copy it. Same idea as make proto-vendor, except that go generate is a known pattern.

I could be convinced of any of the above:

• explicit "you must do go get"
• make proto-vendor copies from api/go into pkg/pillar
• go generate (we can wrap in a make command) copies the files over

[eriknordmark]

I think the minimum requirement is to 1) get rid of the replace directive and 2) preserve or fully provide a new developer workflow for EVE API updates. I don't think in #2 we should require two separate commits.

Today for pillar the current working workflow is:

1. add to proto file
2. make proto
3. make proto-vendor
4. add use of new field/enum in pkg/pillar
5. make pkg/piller

For newlogd and edgeview step 3 is different; it could be that (cd pkg/foo; go mod vendor) works but I haven't checked.
We have the option to change step 3 to be something different for pillar and/or the other two packages, but if we do a change it makes sense doing it all at once and not have developers be subject to a sequence of workflow changes in subsequent PRs.

Does that make sense?

[deitch]

Does that make sense?

I understand it. I do not fully agree that two separate commits is wrong. I think it would be very healthy for the eve API to be a standalone resources, one that is thought about and built and, yes, committed, without a stitch of code using it. But I also understand the alternate perspective and can work with it.

If we really do not want two separate commits, then I think a make target that either copies them in or does go generate is the way to go.

As for the others:

• edgeview: this uses an explicit commit, i.e. you actually need to change the go api, commit it, then add it in with go get. There is no replace => ../../api.go in here. It already does what I am proposing here, so it is out of scope.
• newlog: does precisely what pillar does.

I can try and make a separate commit showing the "copy it in" approach. Then we can pick one commit and delete the other.

[deitch]

OK, copy will not work, nor will just generating. We build the protobufs with a specific path in hand, e.g.:

package org.lfedge.eve.attest;
option go_package  = "github.com/lf-edge/eve/api/go/attest";
option java_package = "org.lfedge.eve.attest";

That means the go path must be that, and copying it into pkg/pillar or pkg/newlog will not work.

I do not have a better answer right now.

[eriknordmark]

That means the go path must be that, and copying it into pkg/pillar or pkg/newlog will not work.

It's vendored, not copied to a different path. Why doesn't that work?

[eriknordmark]

I understand it. I do not fully agree that two separate commits is wrong. I think it would be very healthy for the eve API to be a standalone resources, one that is thought about and built and, yes, committed, without a stitch of code using it. But I also understand the alternate perspective and can work with it.

I think we are talking about different things.
I agree it might make sense to separate out the EVE API in a separate repo, and if and when that is done, there would be separate commits in separate repos to first update the API and then use that API. But that is not this PR.

This PR is fixing an issue due to the replace directives.
I do not see why as part of that we need to change the developers workflow; we should try very hard to not do that.

[deitch]

I think we are talking about different things.
I agree it might make sense to separate out the EVE API in a separate repo, and if and when that is done, there would be separate commits in separate repos to first update the API and then use that API. But that is not this PR.

OK. Something for the future.

I do not see why as part of that we need to change the developers workflow; we should try very hard to not do that.

I don't know of a way to do it at this point. Because go-compiled protobufs include the module path in them, they have to be at github.com/lf-edge/eve/api/go, which is going to be looked for in that path. So copying them over into the local module would just be asking it to be in github.com/lf-edge/pkg/pillar/api/go, which doesn't work (and I tried it). We cannot override it by putting it in vendor/, because that must reflect the real outside. Every time we run go mod vendor, it will read go.mod and go.sum and update it. The canonical way to replace it with something else is... replace.

I am at a bit of a loss now. There doesn't appear to be a way to do it.

@eriknordmark unless you see some other way, I am going to close this one out, and we will live with this not nice replace until we break it out.

[eriknordmark]

I don't know of a way to do it at this point. Because go-compiled protobufs include the module path in them, they have to be at github.com/lf-edge/eve/api/go, which is going to be looked for in that path. So copying them over into the local module would just be asking it to be in github.com/lf-edge/pkg/pillar/api/go, which doesn't work (and I tried it). We cannot override it by putting it in vendor/, because that must reflect the real outside. Every time we run go mod vendor, it will read go.mod and go.sum and update it. The canonical way to replace it with something else is... replace.

Not in pkg/pillar/api but in pkg/pillar/vendor/github.com/lf-edge/eve/api.
Are you saying that golang doesn't look in the local vendor directory but instead downloads from github.com? That would defeat the purpose of vendoring.

[deitch]

Are you saying that golang doesn't look in the local vendor directory but instead downloads from github.com

No, certainly not saying that; it would indeed defeat the purpose of vendoring.

I am saying that what is in vendor/ must match what is in go.mod, and if it doesn't, vendoring fails. When you run go mod vendor, i.e. populate it, anything in vendor that is not in go.mod (or upstream of it) is removed, and anything in go.mod is downloaded into there. We cannot override it by putting the api/go stuff in there without first putting it in go.mod, which, if it is not at the right place and commit on the Internet, will only be used if replace exists.

[eriknordmark]

Are you saying that golang doesn't look in the local vendor directory but instead downloads from github.com

No, certainly not saying that; it would indeed defeat the purpose of vendoring.

I am saying that what is in vendor/ must match what is in go.mod, and if it doesn't, vendoring fails. When you run go mod vendor, i.e. populate it, anything in vendor that is not in go.mod (or upstream of it) is removed, and anything in go.mod is downloaded into there. We cannot override it by putting the api/go stuff in there without first putting it in go.mod, which, if it is not at the right place and commit on the Internet, will only be used if replace exists.

Got it.
So we'd need to do a hack like instead of go mod vendor copy to pkg/pillar/vendor/github.com/lf-edge/eve/api so that the compile can find it. Hmm ...

[deitch]

Yep. And hack vendor/modules.txt. And the next time someone runs go mod vendor, it would break it. All in all, asking for trouble.

[eriknordmark]

Yep. And hack vendor/modules.txt. And the next time someone runs go mod vendor, it would break it. All in all, asking for trouble.

No, don't need that. Once the PR with all the pieces is pushed to master things are sane; a go mod vendor will not change anything. By all the pieces I mean

• api/proto generated api/* changes
• pkg/pillar/vendor/github.com/lf-edge/eve/api/go copies from api/go
• code in pkg/pillar to use the new API

[eriknordmark]

Basically with this Makefile hack (which should be commented) the existing workflow works:

index ed2661135..463ca2f90 100644
--- a/Makefile
+++ b/Makefile
@@ -696,6 +696,11 @@ cache-export-docker-load-all: $(LINUXKIT) $(addsuffix -cache-export-docker-load,
 
 proto-vendor:
        @$(DOCKER_GO) "cd pkg/pillar ; go mod vendor" $(CURDIR) proto
+       @$(DOCKER_GO) "cp -rp api/go pkg/pillar/vendor/github.com/lf-edge/eve/api/" $(CURDIR) proto
+       @$(DOCKER_GO) "cd pkg/newlog ; go mod vendor" $(CURDIR) proto
+       @$(DOCKER_GO) "cp -rp api/go pkg/newlog/vendor/github.com/lf-edge/eve/api/" $(CURDIR) proto
+       @$(DOCKER_GO) "cd pkg/edgeview ; go mod vendor" $(CURDIR) proto
+       @$(DOCKER_GO) "cp -rp api/go pkg/edgeview/vendor/github.com/lf-edge/eve/api/" $(CURDIR) proto
 
 proto-diagram: $(GOBUILDER)
        @$(DOCKER_GO) "/usr/local/bin/protodot -inc /usr/include -src ./api/proto/config/devconfig.proto -output devconfig && cp ~/protodot/generated/devconfig.* ./api/images && dot ./api/images/devconfig.dot -Tpng -o ./api/images/devconfig.png && echo generated ./api/images/devconfig.*" $(CURDIR) api

@naiming-zededa when I try this I see that go mod vendor does not update the vendor directory in pkg/edgeview; same if I do go mod vendor manually in pkg/edgeview. Is there something different between it and newlog?

[naiming-zededa]

@eriknordmark not sure. will this be related to 'go.mod' name resolvable in the other PR?

[deitch]

@eriknordmark do you mind fixing the triple-backticks on your makefile? It is difficult to read.

[deitch]

What are you proposing? That we run go mod vendor, and then afterwards cp everything over? We do still need to fix vendor/modules.txt.

The reason it concerns me is that standard commands, like go mod vendor, will break it afterwards.

[eriknordmark]

How about you

1. Remove the now useless/confusing proto-vendor taget from the Makefile
2. Document the new workflow which is to first commit to api, then to vendor in the new API, then to use it in a separate commit
3. Document a way for a developer to test compiling using the new API (which is basically cp -rp api/go/* pkg/$PKG/vendor/github.com/lf-edge/eve/api/go/)
4. Send a heads up to the lf-edge EVE list about this.

Then we can proceed with this PR.

[deitch]

Remove the now useless/confusing proto-vendor taget from the Makefile

Is it useless? It calls go mod vendor in pkg/pillar, which still is useful.

Document the new workflow which is to first commit to api, then to vendor in the new API, then to use it in a separate commit
Document a way for a developer to test compiling using the new API (which is basically cp -rp api/go/* pkg/$PKG/vendor/github.com/lf-edge/eve/api/go/)

Adding.

[deitch]

Added those docs. Take a look now @eriknordmark

[eriknordmark]

I'm trying to understand the intended sequence of changes for both api and libs which minimizes intermediate approaches for the EVE developes. What I undderstand is:

1. Request creation of github.com/lf-edge/eve-{api,libs}
2. Communicate on EVE mailing list(s) and slack the upcoming change to developer flow
3. Some set of PRs, including this one.
4. Copying the content/history from eve/libs to eve-libs and from eve/api to eve-api
5. Changing all users of eve/libs and eve/api to use the new import paths (including controllers). That will mean at least one PR for EVE, plus adam/eden.
6. Send out a message to EVE mailing lists and slack indicating that the new workflow is in place.
7. Delete the content (and history?) from eve/libs and eve/api.

Anything I'm missing? Any additional EVE changes we need in step 3?

[eriknordmark]

I'm trying to understand the intended sequence of changes for both api and libs which minimizes intermediate approaches for the EVE developes. What I undderstand is:

1. Request creation of github.com/lf-edge/eve-{api,libs}
2. Communicate on EVE mailing list(s) and slack the upcoming change to developer flow
3. Some set of PRs, including this one.
4. Copying the content/history from eve/libs to eve-libs and from eve/api to eve-api
5. Changing all users of eve/libs and eve/api to use the new import paths (including controllers). That will mean at least one PR for EVE, plus adam/eden.
6. Send out a message to EVE mailing lists and slack indicating that the new workflow is in place.
7. Delete the content (and history?) from eve/libs and eve/api.

Anything I'm missing? Any additional EVE changes we need in step 3?

[deitch]

Overall, I think you are correct. I would not delete the history, just the content.

I want to get this PR in first, i.e. doing it locally, and then one each for eve-api and eve-libs. Cleaner separation.

Is there anything else we need for this PR on its own?

[eriknordmark]

Is there anything else we need for this PR on its own?

I'd like to see the heads up email and slack message being sent before we merge this PR.
And that message should outline the steps we are going through to separate the pieces.

[deitch]

I'd like to see the heads up email and slack message being sent before we merge this PR.

Sure thing.

[eriknordmark]

LGTM but we need a heads up message to the EVE developers on email and slack.

[deitch]

Email and slack out.