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

Replace travis job with github action. #66

Merged
merged 3 commits into from
Oct 4, 2020
Merged

Replace travis job with github action. #66

merged 3 commits into from
Oct 4, 2020

Conversation

jmrtt
Copy link
Contributor

@jmrtt jmrtt commented Oct 3, 2020

Created github action that performs the same tasks as travis job.

Note that the badge in Readme.md is already pointing to https://github.com/awalterschulze/gographviz to work correctly when merged.

@jmrtt jmrtt mentioned this pull request Oct 3, 2020
Copy link
Owner

@awalterschulze awalterschulze left a comment

Choose a reason for hiding this comment

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

Yeah, this is very much what I wanted :D
Thank you so much for doing this <3

Should the github action have run on this pull request already?

find . -type f -name '*.go' | xargs goimports -w

test:
go test ./...

travis:
Copy link
Owner

Choose a reason for hiding this comment

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

Is it possible to change this to action instead of travis and then call this from the github action.
That way it is possible to reproduce the github action locally before pushing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a really good idea, I forgot about the possibility to run locally...

In order to have separated steps in github action, I will add different rules to Makefile (dependencies, build and checkers) and an action one that calls the others.

Github actions also run on pull_request.
Add help option to Makefile.
@jmrtt
Copy link
Contributor Author

jmrtt commented Oct 4, 2020

Should the github action have run on this pull request already?

It is configured to run on: push, so it will only run when is pushed to the repository. It makes sense to also run on pull request, so I will change to on: [push, pull_request].

help:
@grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}'

regenerate: ## Re-generate lexers and parsers and pass through goimports
Copy link
Owner

Choose a reason for hiding this comment

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

This looks great 👍

I do not see the regenerate step in github actions.
The git diff --exit-code is also used to check that the newest generated code was checked in.

Otherwise it looks amazing :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, missed regenerate. ☹️

One question, when I execute make regenerate, the created imports for errors and token are local paths. I was expecting import with github.com/jmrtt/gographviz or better yet with github.com/awalterschulze/gographviz. So I'm missing something here.

The import after running make regenerate looks like "home/roquette/gographviz/internal/token" and goimport gives an error as expected. What I'm doing wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that is possible to specify the package in gocc command-line using -p. But it was being ignored since the output is going to ./internal due to https://github.com/goccmack/gocc/blob/65cbc60dd30507e085bed479695e11aaf6fe9c2e/internal/config/config.go#L184

So, I changed the makefile to pass the package as parameter and generate inside internal folder (cd internal; gocc -zip -p github.com/awalterschulze/gographviz/internal ../dot.bnf). Please check if this is the best way to perform this in my last commit.

With this, I updated the generated code and is possible to check using git diff --exit-code.

Last run of the action can be found in: https://github.com/jmrtt/gographviz/runs/1205227899

Copy link
Owner

@awalterschulze awalterschulze Oct 4, 2020

Choose a reason for hiding this comment

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

Aha I think the problem is the GOPATH variable needs to be set.
I think gocc uses the GOPATH to generate the paths.

I think travis used to set this as part of its default go setup

Copy link
Owner

Choose a reason for hiding this comment

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

I think with setting the GOPATH we should be able to get away with the old gocc -zip -o ./internal/ dot.bnf which should then also remove the generated txt files, I hope at least.

Copy link
Owner

@awalterschulze awalterschulze Oct 4, 2020

Choose a reason for hiding this comment

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

Yes it seems it uses the go/build package to figure out the default package
https://github.com/goccmack/gocc/blob/master/internal/config/config.go#L205

I think I still remember encouraging this idea.

Copy link
Owner

Choose a reason for hiding this comment

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

Thank you for putting in this much effort.
I think this work is needed and I do appreciate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the all the information! 😄 I will revert my last commit and try setting GOPATH to have gocc -zip -o ./internal/ dot.bnf back. I'll let you know when have a new iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With GOPATH and correct location, only token.go was updated by regenerate. Generated txt files didn't appear.

So, we already have the regeneration step and github actions working correctly (needed to set GOPATH and working directory to have it working).

Last run of github actions with regenerate: https://github.com/jmrtt/gographviz/runs/1205882765

Copy link
Owner

Choose a reason for hiding this comment

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

Amazing work.

Well done, this looks great.

I think with time of not having a working CI, the token.go got outdated some how. So including in the pull request makes sense :)

@jmrtt
Copy link
Contributor Author

jmrtt commented Oct 4, 2020

Should the github action have run on this pull request already?

It is configured to run on: push, so it will only run when is pushed to the repository. It makes sense to also run on pull request, so I will change to on: [push, pull_request].

Apparently, from what I found, Github action will only run when the workflow is accepted in the repository to prevent abuse in PR. So, when this change is accepted, the next PR will have actions running.

The last run on my fork: https://github.com/jmrtt/gographviz/runs/1205050026

@awalterschulze awalterschulze merged commit a1621d0 into awalterschulze:master Oct 4, 2020
@awalterschulze
Copy link
Owner

Great job it is working :D <3
I have created a test pull request.
#67

Thank you so much.
I have been waiting for this for a long time and it is great to finally have it in place.
It is also a great template for how update my other go projects that are still on travis:

  • gocc
  • goderive
  • gominikanren

If you are interested, you are welcome to do it, otherwise I will probably copy your template in a few months.

@jmrtt
Copy link
Contributor Author

jmrtt commented Oct 4, 2020

Glad that I could help! 😄 And thanks for being so helpful! I will take a look at your other projects and issues.

@awalterschulze
Copy link
Owner

No pressure. Either way, I am already very thankful.
You did all the good work :)

@utkarsh2102
Copy link

Hii,

Thanks, both, for all your work! ❤️

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.

3 participants