-
Notifications
You must be signed in to change notification settings - Fork 99
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
add github action to ensure that ECP-ST-CAR always builds #122
base: master
Are you sure you want to change the base?
Conversation
d9f2b27
to
a183919
Compare
Thanks for the contribution. I was holding out on forcing a pre-checkin build, since most mistakes are fairly simple to fix, and I don’t want to impose too many barriers for people to submit contributions. I will work on making the build clean and then merge the action to see if it doesn’t make life too hard for contributors. Thanks again. |
You can leave it as a non-required action and it’ll tell you whether the build works but it won’t prevent merging. That would at least let you know when a fix is needed. Right now the build is broken for people who check it out and try to see their updates, which is also frustrating. One thing that may interest you is that unless the PR creator disables it, you can push to pull request branches. So you can fix the build before merging — it doesn’t have to be the contributor. |
Thanks. Good ideas. And very relevant point about breakage inhibiting other good behavior. |
This adds a [github action](https://help.github.com/en/articles/workflow-syntax-for-github-actions) that builds the document on every pull request. The goal is to ensure that it builds before merging.
a183919
to
e5021b2
Compare
I've rebased this on |
I am still a bit reluctant to enforce this gate because I can fix most issues easily. Are you seeing a upper/lower case filename issue when you try to build? I am building the latest without issues on my Mac but the Mac filesystem is case insensitive and the xSDK commits have had inconsistencies. What are you seeing? |
I plan to merge this PR after the current round of updates. I don't want to change the workflow while we are getting last-minute changes. |
It seems to work now -- I guess whatever it was got fixed. IIRC it was a missing file, though it may be that I had old dependency information when I tried the build. I saw a different error today and had to do a I don't have super strong feelings about this PR -- I don't mind if you close it as really it's about getting ECP changes in quickly and ultimately it's your document :). I do think it's nice to be able to work on the document and know that I can preview things like pictures by building. The past couple times I've just submitted untested LaTeX and figured you'd work out the issues, but I didn't bother building myself. |
Thanks. I am glad things are working for you. I thinking it is a good idea to integrate this PR before the next iteration. |
This adds a github action that builds the document on every pull request. The goal is to ensure that it builds before merging.
@maherou This is working but currently the build of the ECP-ST-CAR is broken, so it will show errors until that is fixed.
You can see the output for this PR's action here.