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 misleading $Id$ embedded into version string #121

Closed
wants to merge 1 commit into from

Conversation

alexmv
Copy link
Contributor

@alexmv alexmv commented Aug 31, 2020

The $Id$ which can be auto-expanded in files via the ident
attribute does not function the same as the old CVS $Id$ keyword.
In CVS, the keyword expansion was updated on every commit, to contain
the current commit id. In git, it is expanded with the identifier of
the blob it is found in. That is, previous to this commit, the
$Id$ (and hence the reported "version hash") was
f220e479c5d8d85c7b753e95dc5fe0b67bbfbd38 -- and had been since the
file was changed last, in f386360.

Remove the misleading hash, and attendant git attributes file. It
will frequently mislead callers that the version has not changed
when, in reality, it has.

Git itself does not have a way to embed "the current commit hash" into
a file in a way that is updated whenever the commit changes. Nor does
Go natively have a way to embed it into the binary at build time,
though this may change in the future[1].

As alluded to in that ticket, most projects elect to pass in the
build-time commit information via something like:

GIT_COMMIT=$(git rev-parse HEAD)
go build -ldflags "-X main.gitCommit=$(GIT_COMMIT)"

However, smokescreen does not currently have any build system external
to go build which could embed the above logic. Rather than choose a
build system and introduce a new dependency, remove the misleading
hash entirely.

[1] golang/go#37475 and also
golang/go#29228

The `$Id$` which can be auto-expanded in files via the `ident`
attribute does not function the same as the old CVS `$Id$` keyword.
In CVS, the keyword expansion was updated on every commit, to contain
the current commit id.  In git, it is expanded with the identifier of
the _blob it is found in_.  That is, previous to this commit, the
`$Id$` (and hence the reported "version hash") was
f220e479c5d8d85c7b753e95dc5fe0b67bbfbd38 -- and had been since the
file was changed last, in f386360.

Remove the misleading hash, and attendant git attributes file.  It
will frequently mislead callers that the version has not changed
when, in reality, it has.

Git itself does not have a way to embed "the current commit hash" into
a file in a way that is updated whenever the commit changes.  Nor does
Go natively have a way to embed it into the binary at build time,
though this may change in the future[1].

As alluded to in that ticket, most projects elect to pass in the
build-time commit information via something like:
```
GIT_COMMIT=$(git rev-parse HEAD)
go build -ldflags "-X main.gitCommit=$(GIT_COMMIT)"
```

However, smokescreen does not currently have any build system external
to `go build` which could embed the above logic.  Rather than choose a
build system and introduce a new dependency, remove the misleading
hash entirely.

[1] golang/go#37475 and also
    golang/go#29228
@alexmv
Copy link
Contributor Author

alexmv commented Aug 31, 2020

Related to this, it'd be great to see a version bump and associated git tag! :)

@hans-stripe
Copy link
Contributor

I learned something about git today! I'm going to talk to with my team and figure out what we want to do about versioning since the status quo isn't great.

@hans-stripe
Copy link
Contributor

Going to close this since #123 does more-or-less the same thing! Thanks for opening up this PR 😄

@hans-stripe hans-stripe closed this Sep 3, 2020
@alexmv
Copy link
Contributor Author

alexmv commented Sep 3, 2020

Great!

I would have preferred y'all use my commit message which explained a bit more why the previous thing didn't work -- if only so someone else stumble across it and also have the I-learned-something-about-git moment.

@alexmv alexmv deleted the version-string branch September 3, 2020 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants