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

add continuous integration (CI) for linux build #10

Merged
merged 1 commit into from Feb 19, 2023

Conversation

atsju
Copy link
Collaborator

@atsju atsju commented Feb 15, 2023

This will automatically lead to a build on each PR for linux. Thanks @gr5 for having shared your dockerfile for building.
If build fails, we know we must not merge.
See github actions documentation to learn more. You have 2000m free per month on server for automated build; After that either you pay or it does not build anymore; But the free plan should be more than enough here.

I also added a .gitignore file to avoid adding unneeded files by error

I will add windows build in different PR because it's much more complex.
And the maybe continuous deployment (CD) to male release from a simple tag. But this is not a promise.

@gr5
Copy link
Collaborator

gr5 commented Feb 15, 2023

It wasn't my dockerfile - it was @brave-ulysses but you are welcome anyway. :)

@gr5 gr5 self-requested a review February 15, 2023 21:05
.gitignore Outdated Show resolved Hide resolved
@gr5
Copy link
Collaborator

gr5 commented Feb 15, 2023

So if it takes 1 minute to do a make... This will make the "merge pull request" button fail after one minute? What is the user experience going to be like? I guess I should watch a youtube video of it happening? Have to start dinner now.

@atsju
Copy link
Collaborator Author

atsju commented Feb 15, 2023

The user experience is very good and depends also a bit if your branch protection rules. This will decide if you have only a red/green indicator or if you will be denied merge explicitely.

When PR is opened, build starts, there is a yellow line about her build in progress just above merge button that may or not be greyed out. When build is complete it goes green or red. Of course all details are available. Please go to my forked repo in PR section to see the result of a successful build with this action

.gitignore Outdated Show resolved Hide resolved
Comment on lines 13 to 16
- run: sudo apt install -y apt-utils build-essential wget
- run: sudo apt install -y qt5-qmake qt5-qmake-bin qt5-assistant qtbase5-dev qtmultimedia5-dev
- run: sudo apt install -y libqt5charts5 libqt5charts5-dev libqt5multimedia*
- run: sudo apt install -y libqt5datavisualization5-dev libqt5datavisualization5
- run: sudo apt install -y libopencv-core-dev libopencv-core4.5d libopencv-dev
- run: sudo apt install -y libqwt-qt5-6 libqwt-qt5-dev
- run: sudo apt install -y libarmadillo-dev libarmadillo10
- run: qmake
- run: make -j4
Copy link

Choose a reason for hiding this comment

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

Why break up the installation of the dependencies into separate commands? You can combine multiple commands into a single run:

Suggested change
- run: sudo apt install -y apt-utils build-essential wget
- run: sudo apt install -y qt5-qmake qt5-qmake-bin qt5-assistant qtbase5-dev qtmultimedia5-dev
- run: sudo apt install -y libqt5charts5 libqt5charts5-dev libqt5multimedia*
- run: sudo apt install -y libqt5datavisualization5-dev libqt5datavisualization5
- run: sudo apt install -y libopencv-core-dev libopencv-core4.5d libopencv-dev
- run: sudo apt install -y libqwt-qt5-6 libqwt-qt5-dev
- run: sudo apt install -y libarmadillo-dev libarmadillo10
- run: qmake
- run: make -j4
- run: |
sudo apt install -y apt-utils build-essential wget qt5-qmake qt5-qmake-bin qt5-assistant qtbase5-dev qtmultimedia5-dev libqt5charts5 libqt5charts5-dev libqt5multimedia* libqt5datavisualization5-dev libqt5datavisualization5 libopencv-core-dev libopencv-core4.5d libopencv-dev libqwt-qt5-6 libqwt-qt5-dev libarmadillo-dev libarmadillo10
qmake
make -j4

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it makes much difference either way. I think the older way is slightly more readable I suppose?

Choose a reason for hiding this comment

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

@gr5 No, it ultimately doesn't matter, I just don't get why you would install dependencies a few at a time - it's not a pattern that I've seen before 🤷. If for example you wanted to install these dependencies on your local machine, you'd have to copy/paste/run 7 lines instead of 1. Mostly I like to avoid quirks in the code base, it makes it harder for people to read and understand the intent of the original author.

Copy link
Contributor

Choose a reason for hiding this comment

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

hi peyton,

the "apt install" order mostly matched the compile failures on the initial linux compile

clay

Copy link
Collaborator Author

@atsju atsju Feb 16, 2023

Choose a reason for hiding this comment

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

yes maybe we should have 3 run :
run apt-install
run qmake
run make
I agree it makes not much sense to separate apt-installs
And having 3 separate run will increase readability in output (see https://github.com/atsju/DFTFringe/actions/runs/4186601870/jobs/7255312510)

@peytondmurray
Copy link

Hey @atsju, glad to see some others are interested in working on getting DFTF to build on linux. Thanks for this! 🚀

@githubdoe
Copy link
Owner

I don't understand this at all. Why would you want a pull request to cause a build? Seems to complicate a pull request.

@atsju
Copy link
Collaborator Author

atsju commented Feb 16, 2023

I don't understand this at all. Why would you want a pull request to cause a build? Seems to complicate a pull request.

Hi @githubdoe
Doing so is pretty much industry standard for over 10Y, I don't know if it was very common before that but you might know Jenkins for example. Github actions is taking over because it's much easier to maintain and it's free for open source. Normally you also trigger UT (unit test) but at this point we do not have any UT.

I can cite you just a few examples were build in a PR is useful:

  • You build only on windows and you broke linux build without knowing (or the other way)
  • You have dependencies satisfied on your computer but building on a clean environment will not work
  • You missed adding some files in git
  • You have outside collaborators and this builds trust in their modifications
  • You do only a small modification that does not require test but you did not see you broken something unexpected
  • ...

If you never ever had one of these issues I would be very surprised.

I let other complete if I missed something.

Also have a look here if you want to see more details atsju#1 and there for documentation https://docs.github.com/en/actions

image

Edit: this workflow is also the first prerequisite step if we want github to generate the build artifacts. For example people could get the latest master build without you or them having to build DFTF. This is especially useful for windows users (not developer) if you want them to try a master build that has not been released.
It will make a clean build with known versions of dependencies instead of having your untracked local build. Ultimately it will save you time and have better tracking of builds.
Unless artifacts are stored with releases, they are only stored for a limited amount of time.

@githubdoe
Copy link
Owner

Well actually I have never had any of those problems. Probably because I have never made software to work under multiple operating systems. Usually also Windows only if compiled. Yes I can see that what works on one system could break the other. I also have no facility to build any of the unix flavors. Or at least the desire to complicate my development laptop with unix variants. So I can see how a github build could be useful. I just can't imagine how one creates a Qt build environment with all the necessary Qt and other libraries. Sounds like you do. I would not have dreamed Github could do that and have never heard of that capability in my little experience of GitHub.

@atsju
Copy link
Collaborator Author

atsju commented Feb 16, 2023

Thank you for your feedback.
I plan to do the same for windows later, but as you know it's much more complexe to build than on Linux.
I'm a Windows guy but it this specific case it's just a pain.

Please wait 1 day before merging, so I do modification proposed in review above : #10 (comment)

@gr5
Copy link
Collaborator

gr5 commented Feb 16, 2023

Instead of forcing Dale to do PR's also - is there a way he can "run the action" on any branch on github? He could push his working branch to github and then can he run the action on that branch to make sure he didn't miss anything?

In fact can any of us "run the action" on our own personal github accounts? Without doing a PR?

@atsju
Copy link
Collaborator Author

atsju commented Feb 16, 2023

First of all, what you request does not go against having a build per PR. So this feature must be kept.
Also, are you saying Dale should not need to use PR ? Or just not create PR to do the build ? I believe even Dale must create a PR and be reviewed by peers (for tips to him but also for peers to learn).

Once this is said, yes of course you could trigger build on branches. The action can be modified to run on each push on any branch. But remember that build are free with a 2000minutes per month limit. So I don't think we should do that.
Best "basic" compromise is what I did now. But it can be tuned to build branches with specific names or all branches or maybe even on specific comment comments of course.

@atsju
Copy link
Collaborator Author

atsju commented Feb 16, 2023

I think it's even possible to manually trigger the workflow (if correctly written). I will have a look at it then yes you can trigger any build without requiring a PR. Obviously you will ultimately create the PR. Creating the PR and marking it as draft may also fit the requirement

@atsju atsju force-pushed the JST/actionCI branch 2 times, most recently from 20bcf2c to af73763 Compare February 16, 2023 17:25
@atsju
Copy link
Collaborator Author

atsju commented Feb 16, 2023

I did the modifications recommended by @peytondmurray
I added the ability to manually trigger as discused with @gr5
Sorry for force push, I think there is no need to mess up commit history with the details here.

@githubdoe feel free to merge or to make more feedback or ask questions.

@peytondmurray
Copy link

Sorry for force push, I think there is no need to mess up commit history with the details here

Thanks for doing this, I really prefer this workflow anyway - otherwise junk commits pollute the git history. Anyway, great job on this! 🎉

@gr5
Copy link
Collaborator

gr5 commented Feb 16, 2023

I didn't realize this but @atsju assures me that the way he did it, the build will be triggered on:
opening the PR
pushing changes to the PR
Merging the PR
I thought it was only the final one - that's why I asked for ability to run it manually. Manually running it is not necessary now that I understand better.

Copy link
Collaborator

@gr5 gr5 left a comment

Choose a reason for hiding this comment

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

Looks great.

@githubdoe
Copy link
Owner

I added the master branch protection recommended by github. There must be at least one person approving the pull request. Master can not be updated until that. Are there other checks and balances that need to be enabled?

@gr5
Copy link
Collaborator

gr5 commented Feb 17, 2023

I added the master branch protection recommended by github.
Julien sent you a direct email I think with screen shots of his suggestions. I don't understand them all so I won't comment.

@atsju
Copy link
Collaborator Author

atsju commented Feb 17, 2023

I added the master branch protection recommended by github. There must be at least one person approving the pull request. Master can not be updated until that. Are there other checks and balances that need to be enabled?

You might add the check for this action/workflow as mandatory once merged. But what you did is already good and enough for the small team you/we are.
I think you can play with settings later if you see it's needed (if/when we make first errors that could have been avoided).

@atsju
Copy link
Collaborator Author

atsju commented Feb 17, 2023

On more word about the 2000minues limit per month. I searched and searched again why my consumption didn't move during tests. In fact I was wrong and it's 2000min per month per user for private repos only.
This repo being public we have "unlimited" (or reasonably unlimited as in reasonable usage of term of service) build time.

@gr5
Copy link
Collaborator

gr5 commented Feb 19, 2023

I really like this one. I hope Dale pulls it. I already "approved" it.

@githubdoe githubdoe merged commit d2393ac into githubdoe:master Feb 19, 2023
@atsju atsju deleted the JST/actionCI branch August 5, 2023 13:59
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

5 participants