Skip to content

Closes #1319 Report git-hash used to package the distribution in REPL #1571

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

Merged
merged 1 commit into from
Nov 14, 2016
Merged

Closes #1319 Report git-hash used to package the distribution in REPL #1571

merged 1 commit into from
Nov 14, 2016

Conversation

krasinski
Copy link

  • Adds SBT BuildInfoPlugin to know git-hash in runtime
  • Reports git-hash in REPL

@felixmulder
Copy link
Contributor

Hi @krasinski, thanks for the PR. It looks like you're hitting a CI oddity. A rebuild should fix it.

Cheers,
Felix

@felixmulder
Copy link
Contributor

/rebuild

Copy link
Contributor

@felixmulder felixmulder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi again @krasinski - so I've looked over your code and it solves the problem quite elegantly. But there is a caveat. When we're bootstrapping, we don't want to depend on a specific build tool for two reasons.

  1. When doing the bootstrap we want complete control over the classpath and as such usually perform this straight from the command line without invoking sbt (in the future, yes we should have a mechanism for doing the bootstrap with a well guarded classpath in sbt)
  2. Dotty might be compiled with alternative build-tools in the near future (i.e. not sbt)

Therefore I believe it best to go for a solution that does not depend on sbt or provides a default if the plugin has not created the BuildInfo file. If you can provide a fallback - then we're happy to accept this PR.

Cheers,
Felix

@krasinski
Copy link
Author

I will try to do this in another way - by storing the git-hash in manifest file and then reading it back in runtime.

@krasinski
Copy link
Author

Hi @felixmulder, I pushed the new solution, can you review?

@felixmulder
Copy link
Contributor

LGTM, great job @krasinski !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants