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

remove sed in a go1.10 and go1.11 compatible way #6567

Open
wants to merge 13 commits into
base: master
from

Conversation

@solderjs
Copy link
Contributor

commented Apr 10, 2019

This removes sed from the build process, making it pleasurable to build on Mac, Linux, and Windows using only standard Go tooling, like this:
(no sed, make or msysgit required)

go get
go generate -tags 'bindata' ./...
go build -tags 'bindata'

That makes Go the only immediate tooling dependency.

  • doesn't require installing go-bindata at the system level
  • doesn't require sed
  • doesn't require make
  • works with go1.10 (with much blood and tears)
  • works with go1.11
  • ready for go1.13 and all the build/tooling optimizations associated with go.mod going forward

Like #6564, but keeping go1.10 as the minimum required version.

Note: this doesn't remove sed entirely from the Makefile, but it makes it possible to build on all environments equally well with or without the Makefile.

@solderjs solderjs changed the title remove sed in simultaneously go1.10 and go1.11 compatible way remove sed in a go1.10 and go1.11 compatible way Apr 10, 2019

This was referenced Apr 10, 2019

endif

LDFLAGS := -X "main.MakeVersion=$(MAKE_VERSION)" -X "main.Version=$(GITEA_VERSION)" -X "main.Tags=$(TAGS)"

This comment has been minimized.

Copy link
@solderjs

solderjs Apr 10, 2019

Author Contributor

Note that main.Tags is preserved and still uses the same env, but placed more idiomatically in https://github.com/go-gitea/gitea/pull/6567/files#diff-d66c7cbc79283ca2eb2cf49e6a2fd9edR23

@codecov-io

This comment has been minimized.

Copy link

commented Apr 10, 2019

Codecov Report

Merging #6567 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6567      +/-   ##
==========================================
- Coverage   41.67%   41.67%   -0.01%     
==========================================
  Files         414      414              
  Lines       55985    55985              
==========================================
- Hits        23331    23329       -2     
- Misses      29524    29527       +3     
+ Partials     3130     3129       -1
Impacted Files Coverage Δ
modules/options/options.go 96.15% <ø> (ø) ⬆️
modules/public/public.go 74.41% <ø> (ø) ⬆️
models/unit.go 0% <0%> (-14.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6dbd261...0022118. Read the comment docs.

Show resolved Hide resolved public/generate.go Outdated
@zeripath
Copy link
Contributor

left a comment

We shouldn't generate the bindata unless explicitly asked to. I appreciate that almost all of us pass in TAGS="bindata sqlite sqlite_lock_verify" so it seems unnecessary to have to repeatedly specify but there are some very good reasons why you might not want compiled bindata.

Now maybe you could change things so that an empty TAGS leads to generate asking some questions to set TAGS? You'd probably want some way of saving these options too. Of course then you should support nobindata as a build tag.

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@zeripath As far as Tags go, I don't think that makes sense for the non-production use case. The goal here is not to utterly replace the Makefile, but rather to make it so that builds "just work" with standard Go tooling - frustration free.

If someone wants to include TAGS='bindata', they can do so like this:

TAGS='bindata'
go get
go generate -tags "$TAGS" ./...
go build -tags "$TAGS"

(which I would think is intuitive for the type of person who wants to do that)

However, I don't think it should be a hard-stop requirement as it works around the standard "Go way" to do things.

I must have misread the Makefile. I thought that it was already generating bindata all the time, but only including it when the tag was there. I'll take a look into that.

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@zeripath I added // +build bindata to the bindata files. Fixed as requested.

@solderjs solderjs force-pushed the solderjs:nix-sed branch from 566dd8e to 838a2d5 Apr 11, 2019

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@zeripath @sapk Updated as per requests, and passing tests.


func getVersion(gitea, drone, verenv string) string {
// GITEA_VERSION takes priority, always
if 0 != len(gitea) {

This comment has been minimized.

Copy link
@appleboy

appleboy Apr 11, 2019

Member

change to len(gitea) != 0

}

// DRONE_TAG version comes next
if 0 != len(drone) {

This comment has been minimized.

Copy link
@appleboy

appleboy Apr 11, 2019

Member

same above

if 0 != len(drone) {
// Only when DRONE_TAG is set does VERSION takes precedence
// over both DRONE_TAG and the GIT DESCRIPTION
if 0 != len(verenv) {

This comment has been minimized.

Copy link
@appleboy

appleboy Apr 11, 2019

Member

same above

tagcmd := strings.Split("git describe --tags --always", " ")
cmd := exec.Command(tagcmd[0], tagcmd[1:]...)
out, err := cmd.CombinedOutput()
if nil != err {

This comment has been minimized.

Copy link
@appleboy

appleboy Apr 11, 2019

Member

err != nil

}
defer f.Close()
f.Write(src)
}

This comment has been minimized.

Copy link
@appleboy

appleboy Apr 11, 2019

Member

coding style:

if err != nil
@lunny

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@solderjs could you also add some tests for tools and version packages and also change docs?

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@appleboy Putting constants on the left is a good habit to have. It ensures that you'll never have an accidental assignment where it could be prevented. Go has other checks in place so it's less likely, but bradfitz still does it in some of his code, and it's certainly acceptable Go style. Why do it the other way?

@techknowlogick

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

I second @appleboy feedback because it keeps code style consistent in this codebase.

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@lunny These are meta packages for build tooling. They don't have any functional code to test, other than what is already tested by virtue of the build succeeding.

tools.go is an empty file which has no executable code whatsoever. It's purpose is to make go mod tidy include build dependencies in go.mod and go.sum.

version.go is just a main() that runs the shell command to get the version and generates an about.go with the version information.

The output is non-deterministic as it relies on getting the git version from the repository. It wouldn't make sense to build out a fake repository just to test that the behavior of git describe hasn't changed - that's the job of gits own tests.

The about.go cannot be tested because it is an optional override to the built-in variables by the variables built during the build. To test it requires building... which is what is already happening by the existing test and build process.

The only thing that's "testable" is the order in which the environment variables are read - but that behavior was built to mimic what the make file was already doing - which is for the purpose of the build system.

Technically that could be abstracted out into its own library and then a separate main function could be published as a command... but I think that's overdoing it a bit.

I'm not opposed to adding tests, but I don't think that there is anything to test.

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@lunny Just added a test file for testing the priority of the ENV variables, but that's the only thing I think that I can reasonably test here.

I think that after this is merged, someone might consider whether or not the behavior that was written into the Makefile, which is preserved here verbatim, is actually the desired behavior. It seems strange to me, but since it's just build tooling, and it's how it already was, I thought might as well copy it.

Having written tests for it now, however, I question if it really needs to be so complex (and inconsistent).

My guess is that the Makefile was actually incorrect (but happened to not fail from the complexity) and that what was intended was more along the lines of this:

version := os.Getenv("DRONE_VERSION")
if len(version) == 0 {
  version = getGitDescribeVersion()
}
version = strings.TrimLeft(version, "v")

I think that the other stuff regarding GITEA_VERSION and VERSION were intended to be used as local variables but were declared as environment variables by mistake - probably a copy and paste error - as pretty much everything in the Makefile is declared that way also, and probably isn't intended to be, and there would never be a case that causes it to fail.

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@techknowlogick @appleboy

If that's your personal preference I'd be just fine if you went back and changed it to the way you like, but as far as contributions Go I think we should just stick with standard Go code style (which is already quite stringent) and not put additional restrictions on it other than that, except where there's a cost (i.e. bringing in another dependency when an existing dependency already has the same function or a little copying would do).

I think that enforcing typographically and /or lexicographically sorted if statements (if a == b vs if b == a and if nil == a vs if a == nil is a bit on the micro-manage-y side for an open source project in Go.

The process is already confusing - the whole autonomous organization thing and not knowing who has "say" or what "matters" - and I really just want to help deliver value (to Windows users in this case) and get a quick return.

I don't mind iterating on things that are objectively important - like fixing functionality that changed, edge cases on different platforms, failing tests, etc. These are all things I expect to spend time on.

When it comes to subjective things, I'd much prefer it if the people with the strongest opinions would take responsibility to the effect of "This is my personal preference, would you mind if I change this after it gets merged in?" rather than having an ambiguous burden on the person doing the patch.

Does that make sense? Do you see value in that approach? Is there something you really value about the current process that might be helpful for me to understand?

@solderjs solderjs force-pushed the solderjs:nix-sed branch from 26866c9 to 838a2d5 Apr 12, 2019

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@lunny I pulled my test back out because I think it would be better to define what is supposed to be tested, and because it would require adding tooling-specific build tags to existing tests to be tested (and I'm pretty sure that's not what you really want). If it is, I will do it.

@typeless

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

@solderjs In my opinion, it is more about consistency rather than personal preference at this point. It was probably a subjective choice of the original authors (like unknown/lunny etc). Typically, I prefer to follow the style of the surrounding code, even if it were not the best in my own taste. Plus, it'd be a good idea to separate the style changes & features/fixes in different PRs. Or if we want to adopt a new style, we should change the whole codebase (or plan for doing it incrementally).

Just my 2 cents.

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@typeless (cool name, btw)

I don't see in value in consistency for consistency's sake. If the original authors happened to do something a certain way (i.e. lexicographically sort if a == b, or in this case use if a == nil) consistently, there's no virtue to continuing to do it (or to not do it).

Since that's something very local to this specific codebase, and not the Go community at large, and since it literally doesn't matter in terms of readability or code function, why bother? Why add the overhead?

From a pragmatic point of view, it's just one more thing that has to maintained, communicated, etc. If no one actually knows why its done or why it's desirable or preferable, then why canonize it?

In my point of view, consistency in of itself is only valuable when it reduces the need for communication.

And I was about to give an example... but I can't come up with one for Go because all of the things that I would normally choose are already handled by go vet, go lint, go fmt, or one of the other standard Go tooling.

Anyway, in this case, having localized style increases communication cost and work rather than reduce it.

I believe that local styles are important, but I believe they should be used for the sorts of things that reduce documentation and communication cost. Again, Go pretty much already enforces everything I would normally present as on off-the-cuff example.

By design, Go makes it difficult to have these sort of debates. :) Yay for Go the utilitarian pragmatist Go developers 👍.

@lafriks lafriks added the kind/build label Apr 12, 2019

@lafriks lafriks added this to the 1.9.0 milestone Apr 12, 2019

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@appleboy @techknowlogick I've made the requested change, but I would like to continue to have the discussion. I believe like this is an amazing project and my goal is to help make it more accessible to more people and reduce your overhead so that more contributions can make the full loop without any unnecessary friction.

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@lunny I've committed the tests again with the build tag, but I'd still want to know who knows what the sequence of operations is supposed to be (which I guessed above).

@@ -59,6 +59,8 @@ pipeline:
image: golang:1.10 # this step is kept as the lowest version of golang that we support
pull: true
commands:
- go get

This comment has been minimized.

Copy link
@techknowlogick

techknowlogick Apr 13, 2019

Member

go get shouldn't be needed because necessary packages should be in vendor directory already.

This comment has been minimized.

Copy link
@solderjs

solderjs Apr 13, 2019

Author Contributor

Except that go1.10 doesn’t support go.mod and so -mod=vendor won’t work and so it’ll try to fetch from cache instead (which will fail and break the build).

This comment has been minimized.

Copy link
@solderjs

solderjs Apr 13, 2019

Author Contributor

And since the go generate needs the cache to exist, if go1.10 is going to be supported, go get must come before go generate. Otherwise he generate can’t use go run, which would mean that any tool used by go generate would individually need its own go get before go generate could be called. Hence, I say we should drop go1.10 and start preparing for go1.13 instead.

This comment has been minimized.

Copy link
@solderjs

solderjs Apr 13, 2019

Author Contributor

If you don’t understand that, just try the change yourself and read up on go.mod.

This comment has been minimized.

Copy link
@sapk

sapk Apr 13, 2019

Member

@solderjs you can replace commande like go run github.com/jteeuwen/go-bindata/go-bindata by go run ./vendor/github.com/jteeuwen/go-bindata/go-bindata and this go get should not be need.

This comment has been minimized.

Copy link
@solderjs

solderjs Apr 13, 2019

Author Contributor

That’s not compatible with go.mod. In this particular instance it may happen to work because there’s only one file to run, but in general it’s a bad pattern with inconsistent results.

Also, that methodology won’t support passing arguments, IIRC, because go run would take them.

Lets prepare for go1.13 rather than doing things in hacky ways that become more technical debt.

This comment has been minimized.

Copy link
@solderjs

solderjs Apr 13, 2019

Author Contributor

When we drop go1.10 we can drop the go get and replace it with -mod vendor.

This comment has been minimized.

Copy link
@typeless

typeless Apr 18, 2019

Contributor

@solderjs If I understand correctly, Go pre-1.10 still recognizes the vendor directory. Once it is set up (that is, dependencies are pulled in), Go 1.10 will look it up anyway.

It's that Go 1.10 cannot utilize go.mod to manage dependencies. But that's already the case sinec started using modules.

This comment has been minimized.

Copy link
@techknowlogick

techknowlogick Apr 18, 2019

Member

Yes, we want the CI step to fail if deps aren't met, as like @typeless mentioned, go before modules was implemented will check the vendor dir. If this CI step fails it indicates that the vendor dir was not appropriately updated.

@typeless

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

ping @appleboy, your requests have been addressed.
@solderjs there are some remaining Yoda notations in tests.

@solderjs solderjs force-pushed the solderjs:nix-sed branch from 66006ce to 0022118 Apr 18, 2019

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@typeless Updated 'yoda conditions' and rebased.

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