Skip to content
This repository has been archived by the owner on Feb 26, 2019. It is now read-only.

godep save: add -tags flag. #117

Closed
wants to merge 1 commit into from
Closed

godep save: add -tags flag. #117

wants to merge 1 commit into from

Conversation

jnfeinstein
Copy link

I needed this for a project and couldn't find any previous efforts, so I attempted it myself. I based this on the fact that go list seemed to work fine with the -tags flag. I ran it with my project and godep produced the correct outputs. Please let me know if this is useful and/or done correctly.

@jnfeinstein jnfeinstein mentioned this pull request Aug 31, 2014
@jnfeinstein
Copy link
Author

@kr: I'm guessing that you run this show! Any way that I can get feedback on this?

@mauriciosl
Copy link

I too need this :(
I'm using https://github.com/rainycape/magick and it has the backend configurable through -tags

@maxekman
Copy link

Will this be merged? I just added tag based configuration for the storage of my CQRS library https://github.com/looplab/eventhorizon, without the proper tag on godep save the dependencies for the specified storage backend are missing.

@whazzmaster
Copy link

I'm also interested in this PR; I'd much rather use an official release rather than to fork it and apply the patch just to run goose on heroku.

whazzmaster pushed a commit to adorableio/godep that referenced this pull request Jan 15, 2015
This commit is a patch based on tools#117
that adds support for -tags to godep.

It is REQUIRED in order to save the deps for a different platform on
your dev machine; in our case it is to save off the heroku deps
in addition to our application deps in order to pull in and build the
goose migration tool remotely and have it available on heroku.
@jsha
Copy link

jsha commented Jun 19, 2015

For the Let's Encrypt project (letsencrypt.org), we depend on tags to generate the correct dependency graph for godep, so the main branch doesn't work for us. The pull request fixes a critical issue for us, so currently we have to tell developers to fetch and install this branch instead of the default godep. It would be really helpful if you could merge this, @kr!

Thanks,
Jacob

@freeformz
Copy link

@jnfeinstein Please rebase against master and I'll take a look.

This flag can be used to build the dependencies for a project
that uses build tags for conditional compilation.  For example,
`godep save -tags heroku` will produce dependencies for assuming
that the 'heroku' tag is set.

Because package names are passed using an arbitrary length names
arg in pkg.go, I opted to create a new function that accepts tags
in order to minimize the amount of refactoring that would be
needed.  It seems that `go list` works perfectly fine with the
tags flag.

All save tests pass, but I have not yet figured out how to modify
the contents of a file to make a valid test for this code.
@jnfeinstein
Copy link
Author

@freeformz, the branch is rebased. The only conflict was in the usage blurb.

@leehambley
Copy link

We just put an app into production, and we ran into this, we have conditional build-time metrics and analytics that rely on an external package (Rollbar), hidden behind builds tags, godep can't see it.

We've worked around it by importing the Rollbar package to _ in a // +build ignore version of the file, which is less than ideal, since we still have to manually rewrite the import path in our // +build release file.

@freeformz
Copy link

Thanks @jnfeinstein. At this point I think we need to have the new exported method documented and a test case.

@kr
Copy link
Member

kr commented Jun 24, 2015

I have hope that this can be solved without exposing additional knobs. The general approach I would take: godep should find all dependencies, ignoring // +build lines entirely. It should behave as if all build constraints are satisfied when finding imports. #84 has a little bit of discussion on this topic.

@freeformz
Copy link

@kr That probably makes the most sense in most circumstances.

although I do wonder if there is a case where the developer doesn't support building everything with a dep that uses a different build tag then the one they designed for.

@kr
Copy link
Member

kr commented Jun 26, 2015

This approach wouldn't change what gets built, only what code is included in the repository.

@freeformz
Copy link

That is correct, but since it's vendoring everything regardless of // +build it could include something the developer didn't intent to include/support, which could then be triggered with a -tag by the builder.

Not sure if it matters though.

@kr
Copy link
Member

kr commented Jun 26, 2015

could then be triggered with a -tag by the builder

If that's the case, then the alternative is it wouldn't build at all for that builder.

It's still the responsibility of the person running godep save to at least glance at the diff to make sure it's reasonable, regardless of what godep does about tags.

@kr
Copy link
Member

kr commented Jun 26, 2015

I suppose not building at all can be preferable to including unsupported code for an unsupported build configuration. Hmmm.

@freeformz
Copy link

@jsha would it work for you guys if godep just included all the deps, ignoring // +build tags? Thereby punting the tagging off to those building the code later?

IMO this is the best solution.

@freeformz freeformz self-assigned this Sep 4, 2015
@jsha
Copy link

jsha commented Sep 4, 2015

@freeformz: That sounds like the best solution to me as well, thanks!

@freeformz
Copy link

Great I am going to close this then in favor of: #271

@freeformz freeformz closed this Sep 4, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants