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

Regresion: newest version requiring Bazaar to compile #21080

Closed
bltavares opened this issue Apr 23, 2019 · 10 comments
Closed

Regresion: newest version requiring Bazaar to compile #21080

bltavares opened this issue Apr 23, 2019 · 10 comments

Comments

@bltavares
Copy link
Contributor

On a previous PR (#20870), we've removed the dependency on mgo and gocheck hosted on bzr repositories to avoid downstream libraries to also depend on this extra build tool.

A recent commit reintroduced those dependencies: 8a43011

Now, compiling providers with the latest beta does require bzr on the system to download the libs, even tho it's used on terraform.

Terraform Version

  • master
  • v0.12.0-beta2

Expected Behavior

  • Compiling providers with the newest version should not require extra build tools

Actual Behavior

  • Requiring v0.12.0-beta2 to compile a provider to match the compatible plugin version crashes when bzr is not present, when it didn't crash before.
go: launchpad.net/gocheck@v0.0.0-20140225173054-000000000087: bzr branch --use-existing-dir https://launchpad.net/~niemeyer/gocheck/trunk . in /Users/bruno/go/pkg/mod/cache/vcs/f46ce2ae80d31f9b0a29099baa203e3b6d269dace4e5357a2cf74bd109e13339: exec: "bzr": executable file not found in $PATH

Steps to Reproduce

Additional Context

How could we avoid reintroducing the error? Removing bzr from the PR CI would be an alternative?

References

@bltavares bltavares changed the title Regresion Regresion: newest version requiring Bazaar to compile Apr 23, 2019
@apparentlymart
Copy link
Contributor

Hi @bltavares,

I think we will need to admit defeat here. If our dependencies are distributed via a Bazaar repository then we can't avoid needing bzr to install them. The Go tool seems to think that these dependencies are needed, so removing it again would at best just be a temporary solution, and this will re-occur every time we update whatever is transitively depending on these.

The usual answer to this is to make use of the vendor directory, which Terraform already supports. If you build it with -mod=vendor then it should not need to install any additional dependencies:

go install -mod=vendor .

If you are changing the dependencies for another repository (such as a Terraform provider) then you will need to have the tools necessary to access those dependencies. I don't think we can really avoid that.

The longer-term goal here is for the Terraform provider SDK to not be in this repository at all, at instead to move out into its own repository. At that point, the dependencies of Terraform itself will not become transitive dependencies of the providers.

In the mean time, perhaps I can help you out by creating a branch in the terraform-provider-github repository that already has the new version of Terraform vendored, and then you can just build with -mod=vendor to avoid needing to run bzr.

@apparentlymart
Copy link
Contributor

I pushed up a branch sdk-v0.12-beta2 with an updated Terraform SDK.

It looks like a transitive dependency here has also required moving to a new major release of the go-github library, which has breaking changes and so the provider no longer compiles. It looks like there will be some further work here to either figure out what is causing that transitive dependency upgrade and eliminate it, or -- more likely -- to update the provider code to work with the new go-github API.

@bltavares
Copy link
Contributor Author

@apparentlymart that is unfortunate.

It seems that it is completely avoidable if all the dependencies included use go.mod, as go will not bring transitive dependencies on the file, but that does seem like a far away goal.

I've tried again applying the same strategy of removing the failing dependencies, and after running go mod tidy it was not added again. I guess that is a failure mode of go get -u during the transition phase.

I've quickly came up with a test strategy to avoid including bazaar dependencies, which could also expand to avoid hg dependencies if we desire so. It is not on a good commit yet, but my quick test showed some progress: bltavares@614b814

What do you think of this strategy? I do understand if that is not the way to Go, but requiring bzr on Windows is no-go, as it fails even if it is present.

@bltavares
Copy link
Contributor Author

Regarding go-github major release upgrade, there is a PR waiting for it to upgrade, which fixes the breaking changes (https://github.com/terraform-providers/terraform-provider-github/pull/207)

I've already build two other features on top of it (https://github.com/terraform-providers/terraform-provider-github/pull/211 and https://github.com/terraform-providers/terraform-provider-github/pull/212) and I've been using it internally. so far so good.

My next step was going to try to bump it to the beta release, to use HCL2 syntax, but then I've found the bazaar bug again.

@apparentlymart
Copy link
Contributor

apparentlymart commented Apr 24, 2019

I have updated my sdk-v0.12-beta2 branch with two additional commits: one updates the go-github library to v24 and the other incorporates the breaking change fixes from terraform-providers/terraform-provider-github#207.

I was then able to build it without any external dependencies:

go install -mod=vendor .

Looking again at this issue with Bazaar on Windows: from the stack trace, it looks like this error is actually coming from the Bazaar git plugin. I don't know Bazaar well at all, but from a quick read of the source code involved it seems like this plugin is probing https://launchpad.net/mgo/v2 to see if it is actually a git repository rather than a Bazaar repository. Specifically, it looks like it's trying to request https://launchpad.net/~niemeyer/mgo/v2/info/refs?service=git-upload-packcurl (or similar) to see if it behaves like a git HTTP remote would. The Launchpad server then seems to be returning a response which Python's HTTP library considers invalid, causing the IncompleteRead crash.

I assume the idea here is that you can use bzr to work with Git repositories if you happen to really like bzr's UI, but for our purposes here that's pretty superfluous: go get knows how to run git directly, so bzr will only run if the remote repository is already known to go get as a Bazaar repository, in this case seems to happen as a result of a meta tag on the launchpad page:

<meta name="go-import" content="launchpad.net/mgo/v2 bzr https://launchpad.net/~niemeyer/mgo/v2" />

With all of that said, I wonder if it would work to just disable the Git plugin entirely by setting BZR_DISABLE_PLUGINS:

set BZR_DISABLE_PLUGINS=git

Over in bzr-git plugin land I think it'd be better for them to treat any error from probing as "does not support git" and let things continue, but hopefully in the mean time just disabling this plugin will be good enough to unblock.

@bltavares
Copy link
Contributor Author

That is a really thorough writup, thank you so much. I would be able to test disabling the git plugin until the end of the week.

When testing it last time, I've just used the regular Bazaar install on Windows, and I was not even aware of such integration. Disabling it seems like an extra hoop to jump when writing providers, and very likely, vendoring would need to be documented due the pitfall.

I think it would be better to track down which dependency is bringing the bzr version of those dependencies, and either update or fix it upstream. Both code are quite outdated, and it is very unlikely that new dependencies would depend on bzr, specially as the proposal of go mirrors are progressing.

What do you think of updating the dependencies which bring in the bzr version of such dependencies, and moving them to the git mirrors if necessary?

@apparentlymart
Copy link
Contributor

For launchpad.net/gocheck it seems that we're inheriting this dependency from Consul due to the Consul remote state storage backend:

$ go mod why -m launchpad.net/gocheck
# launchpad.net/gocheck
github.com/hashicorp/terraform/backend/remote-state/consul
github.com/hashicorp/consul/api
github.com/hashicorp/consul/api.test
github.com/hashicorp/serf/serf
github.com/hashicorp/go-msgpack/codec
github.com/hashicorp/go-msgpack/codec.test
labix.org/v2/mgo/bson
labix.org/v2/mgo/bson.test
launchpad.net/gocheck

We can see from here that the mgo dependency is coming from here too. It seems that go-msgpack is using it for bson support, but only in codepaths that Terraform itself doesn't depend on.

However, I noticed that we were pinned to an older version of go-msgpack that wasn't yet enabled for Go Modules, so I've opened #21082 to upgrade to a newer version that has identical code but introduces an explicit go.mod file, which seems to have allowed the go tool to now determine that indeed these two dependencies are not required.

I'm not going to promise that there will never be a dependency from bzr in Terraform ever again, but at least for now we can eliminate these two in a way that should avoid them being re-introduced by subsequent upgrades elsewhere. It may be good to open an issue on bzr-git to see if they can fix this bug, but I don't have a launchpad account and I'm not sufficiently motivated to get one now that we've got a potential short-term fix for the problem.

@bltavares
Copy link
Contributor Author

Meanwhile, I was trying out the suggestion of disabling bzr-git. I've installed the Compact installation version of Bazaar, and I was able to install the dependencies properly. This installation mode does not install any of the plugins, so it didn't trigger the bug.

I've tried using depth, but the dependency tree output don't even show up for those dependencies.

It is very likely to be one of the issues of managing dev tools on go-msgpack when it interact with go mod requires.

(Sharing those for the sake of documentation.)

#21082 looks like a good solution, and I understand it is impossible to promise no new bzr dependencies will show up, yet it seems unlikely. I've seem many dependencies using gopkg.in versioned mirror.

Thank you so much for how thorough your comments were. Landing the update version would help a lot when Terraform 0.12 is announced and providers need to upgrade. It will save a lot of frustration for those coding on Mac and Windows (and some Linux users), which often don't even have Bazaar installed.

@bltavares
Copy link
Contributor Author

Fixed by #21082

@ghost
Copy link

ghost commented Jul 26, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jul 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants