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

promql Go import problem: use of vendored package not allowed #1720

Closed
fatih opened this Issue Jun 9, 2016 · 16 comments

Comments

Projects
None yet
6 participants
@fatih
Copy link
Contributor

fatih commented Jun 9, 2016

What did you do?

I've tried to import the github.com/prometheus/common/model package so I can construct a promql.AlertStmt from scratch. Here is the demo code that I'm using:

package main

import (
    "github.com/prometheus/common/model"
    "github.com/prometheus/prometheus/promql"
)

func main() {
    _ = &promql.AlertStmt{
        Annotations: model.LabelSet{},
    }
}

Building this package is not possible because promql uses a vendored package and thus I've got this error:

./demo.go:10: cannot use "github.com/prometheus/common/model".LabelSet literal (type "github.com/prometheus/common/model".LabelSet) as type "github.com/prometheus/prometheus/vendor/github.com/prometheus/common/model".LabelSet in field value

This is ok, because they really are two different packages (one is vendored, and the other one is pulled by me and put into GOPATH), but if you go and use the vendored import path:

package main

import (
    "github.com/prometheus/prometheus/promql"
    "github.com/prometheus/prometheus/vendor/github.com/prometheus/common/model"
)

func main() {
    _ = &promql.AlertStmt{
        Annotations: model.LabelSet{},
    }
}

Now you get the error:

package demo
    imports github.com/prometheus/prometheus/vendor/github.com/prometheus/common/model: use of vendored package not allowed

What did you expect to see?

I want to be able to create a custom promql.AlertStmt with the Annotations field

What did you see instead? Under which circumstances?

As described above, it's not possible to create such a value

Environment

go version go1.6 darwin/amd64
  • System information:
Darwin Fatihs-MacBook-Pro.local 15.5.0 x86_64
  • Prometheus version:

I'm using latest master with Git SHA a08943e6d347119bfb1b50e738fca577e5733290

@fatih

This comment has been minimized.

Copy link
Contributor Author

fatih commented Jun 9, 2016

A local solution for me would be moving the vendor folder content to GOPATH and remove the vendor folder.

Also here is a conversation I had with dave cheney https://twitter.com/fatih/status/740883554264096772

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Jun 9, 2016

The problem here is that you are using the promql package as a library, which it was not intended to so far. Or in other words: We vendor in the prometheus/prometheus repo because it is not considered to contain libraries (in contrast to prometheus/common).
More and more people are using parts of prometheus/prometheus as libraries, though.
We have to look into how to make that easier.

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jun 9, 2016

Yes it is intended to be used as a library. I fought hard to design it as one and it is in wide use by several companies.

It is arguably a design flaw in the vendoring mechanism and as far as I know there are vendoring tools handling thise case by stripping vendoring from packages you vendor yourself.

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Jun 9, 2016

Wouldn't promql then be a candidate to move into it's own repo? Or into prometheus/common? It's not easy for externals to spot which part of an application repo are meant as libraries and which not.

WRT to nested vendoring: Sometimes, nested vendoring is desired (for any identifiers that do not show up in the API of the library and are therefore completely contained). It would be hard for a vendoring tool to distinguish between the two cases.

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 9, 2016

PromQL is pretty tightly bound to Prometheus, and we're not willing to offer any guarantees around API stability, so I think where it is is fine.

@fatih

This comment has been minimized.

Copy link
Contributor Author

fatih commented Jun 9, 2016

I think the better solution would be move out promql into a separate repo and then vendor it. That way everyone can make use of it. I'm not sure though what the better solution would be, as @brian-brazil mentioned just looking at the code today, they indeed a very tightly bound.

But just a side note, the way promql was laid out, with it's documentation and co, It was intended from the beginning as a library as @fabxc mentioned. It's a joy to use it and so far I'm really happy with it. We use it here at DigitalOcean for our internal systems and was quite happy to find that we could use the parser and manipulate the AST.

That's why I think the stability should be guaranteed

@brian-brazil

This comment has been minimized.

Copy link
Member

brian-brazil commented Jun 9, 2016

That's why I think the stability should be guaranteed

That's a completely separate discussion.

@fatih

This comment has been minimized.

Copy link
Contributor Author

fatih commented Jun 9, 2016

@brian-brazil well you said it's not guaranteed and I said it should. Not sure why you wrote this comment. Happy to discuss it in another issue.

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Jun 9, 2016

So yes, nothing is wrong with the vendoring in prometheus/prometheus. We could close this issue and open a new one titled "Move promql into a different repository?", or we could simply rename this issue and have the discussion here (as it has already happened anyway).

@fabxc

This comment has been minimized.

Copy link
Member

fabxc commented Jun 9, 2016

There's no such thing as "meant as library" if we design each package in a manner that it exposes a well-designed API for the behavior it aims to implement.
Locality of packages is merely to simplify making modifications as we need them ourselves (i.e. no constant revendoring) and to indicate to potential users the API instability that comes with it.

I'm not comfortable moving PromQL out at this point. It has an API that makes it useful for external use cases but is not there yet for a stable package.

I think we have to count on vendor tooling here to work around the general issue. Especially since this is not PromQL specific and will come up again and again. Having 40 repos with distinct libraries for a project does not scale for any Go project out there.

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Jun 9, 2016

OK, vendoring declared as "still unsolved", and this issue declared closed. :)

@fatih

This comment has been minimized.

Copy link
Contributor Author

fatih commented Jun 13, 2016

Thanks for the clarification. I've created a custom script to extract the promql package into a new import path which can be used by any third party package for a given prometheus version. Until this is solved by the team itself I'm going to use it.

@kardianos

This comment has been minimized.

Copy link

kardianos commented Jun 15, 2016

@beorn7
In order to host both a reproducible cmd build and have packages that can be imported elsewhere, I see two options in general for packages and repos to take in Go:

  1. Move the vendor folder in the cmd/ folder. Then after updating a package, either periodically, or in a pre-commit hook or similar, run cd cmd && govendor update github.com/prometheus/prometheus/... which will copy over the package files into the vendor folder makeing a clean cmd build.
  2. Have packages that import the prometheus use a tool like govendor that will also flatten the vendor imports out.
  3. (Not recommended) You could also add a line vendor/*/ in the .gitignore file (and not check-in the dependencies) and add a post update hook in git to run govendor sync to copy the dependencies in to the vendor folder.

I'm working on having other options for the future, but that's about where we are right now. One of the "benefits" of option (1) is you could decouple your cmd release from your importable packages schedule.

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented Jun 15, 2016

  1. Move the vendor folder in the cmd/ folder. Then after updating a package, either periodically, or in a pre-commit hook or similar, run cd cmd && govendor update github.com/prometheus/prometheus/... which will copy over the package files into the vendor folder makeing a clean cmd build.

This is essentially "self-vendoring" and would require the constant re-vendoring @fabxc mentioned before. It's equivalent to moving the cmd directory into its own repo, where then everything, including prometheus/prometheus, is vendored. It has the advantage of not requiring one repo per package, but it has a great potential of confusion.

  1. Have packages that import the prometheus use a tool like govendor that will also flatten the vendor imports out.

That's what @fabxc meant by "I think we have to count on vendor tooling here to work around the general issue."

noonat added a commit to upsight/franz that referenced this issue Feb 17, 2017

Removes vendor folder
This was potentially causing type conflicts in consumer libraries.
See prometheus/prometheus#1720

noonat added a commit to upsight/franz that referenced this issue Feb 17, 2017

Removes vendor folder
This was potentially causing type conflicts in consumer libraries.
See prometheus/prometheus#1720

noonat added a commit to upsight/franz that referenced this issue Feb 17, 2017

Removes vendor folder
This was potentially causing type conflicts in consumer libraries.
See prometheus/prometheus#1720

noonat added a commit to upsight/stop that referenced this issue Feb 17, 2017

Removes vendor folder
This was potentially causing type conflicts in consumer libraries.
See prometheus/prometheus#1720
@tnachen

This comment has been minimized.

Copy link

tnachen commented Jan 15, 2018

@fatih is your script publically available? Having the exact same issue at the moment

@lock

This comment has been minimized.

Copy link

lock bot commented Mar 23, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 23, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.