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

Redo makefile to inject version, fix snapshots, update docs #129

Conversation

StevenACoffman
Copy link
Member

@StevenACoffman StevenACoffman commented Oct 3, 2021

I broke this out of #127 so this is my first stacked diff.

  • Update snapshots to make tests pass
  • Redo Makefile and inject version into building/installing
  • Update documentation for injecting version into go run-ing
  • Add makefile niceties like make help and advice from https://tech.davis-hansson.com/p/make/

This presumes that we want to continue to maintain a Makefile. If we want to use Mage I would be more than happy to do that instead, as Mage would probably work better on a windows machine that didn't have WSL installed.

Signed-off-by: Steve Coffman <steve@khanacademy.org>
@StevenACoffman StevenACoffman changed the base branch from main to benkraft.version-in-generated October 3, 2021 17:24
@StevenACoffman StevenACoffman changed the title steve.version in generated Redo makefile to inject version, fix snapshots, update docs Oct 3, 2021
@csilvers
Copy link
Member

csilvers commented Oct 3, 2021

I'll wait to see if ben wants to comment on this -- in general I'm happy to keep ben as the main maintainer if he wants to be! I'm not sure what thoughts he had.

I will point out that this doesn't do anything for our use at Khan, which runs genqlient using go run. In general, I wonder if we should just go with the simpler solution of hard-coding the version number in the library, and using that. It's probably already hard-coded somewhere.

@StevenACoffman
Copy link
Member Author

@benjaminjkraft What are your thoughts?

Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

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

Perhaps I misunderstood -- you could explain more in the PR message -- but this seems very incomplete since the recommended way to run genqlient is go run? I don't think telling people to do //go:generate go run ... -ldflags ... really worth it, I think we should either fix the runtime.BuildInfo method or just put a version const in the source.

@csilvers csilvers requested review from csilvers and removed request for csilvers October 6, 2021 17:20
Copy link
Member

@csilvers csilvers left a comment

Choose a reason for hiding this comment

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

(Just to get off my queue)

Copy link
Contributor

@dnerdy dnerdy left a comment

Choose a reason for hiding this comment

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

(Also requesting changes to get this PR off my queue.)

@benjaminjkraft benjaminjkraft mentioned this pull request Feb 15, 2023
@benjaminjkraft
Copy link
Collaborator

1.18 adds simpler ways to do this!

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.

4 participants