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

Build binaries using Github actions. #181

Merged
merged 45 commits into from
Sep 20, 2020
Merged

Build binaries using Github actions. #181

merged 45 commits into from
Sep 20, 2020

Conversation

remgodow
Copy link
Contributor

@remgodow remgodow commented Sep 7, 2020

A proposal of fix for #46.
Saves debug binaries for all OS in Github Action Artifact.
Branched from Windows pull request to test Windows OS builds.
Based on ripgrep

Remigiusz Godowicz and others added 28 commits August 16, 2020 23:03
…plementation based on sysinfo and netstat2.
# Conflicts:
#	Cargo.lock
# Conflicts:
#	Cargo.lock
#	Cargo.toml
# Conflicts:
#	Cargo.lock
#	Cargo.toml
Copy link
Owner

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

Awesome. :) I left some minor comments, and after we merge the opensockets PR I think we can definitely go ahead with this one and even use this instead of travis.

pull_request:
push:
branches:
- master
Copy link
Owner

Choose a reason for hiding this comment

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

We use main as our main branch

strategy:
matrix:
build:
# We test ripgrep on a pinned version of Rust, along with the moving
Copy link
Owner

Choose a reason for hiding this comment

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

ripgrep => bandwhich :)

push:
branches:
- master
- actions
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 we can remove this once we're ready to merge and don't need to debug anymore, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly

rust: stable
- build: nightly
os: ubuntu-18.04
rust: nightly
Copy link
Owner

Choose a reason for hiding this comment

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

Right now on travis, our nightly branch is allowed to fail and we just use it as an FYI. Is there a way to do this here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not now unfortunately. I'm not familiar with setting up checks for pull requests, but I guess that we could do ci action with only stable as a gate for pull requests and ci_nightly which would build but would not guard anything.

@imsnif
Copy link
Owner

imsnif commented Sep 10, 2020

Hey @remgodow, let me know if there's anything I can pick up on this side. Maybe I can work on the binary release part a bit? (eg. keep our naming convention, make it build/release a generic musl linux file, etc.)?

Also, what do we have left after this? (I know I've asked before somehwere, but I can't seem to find where just now - sorry :) ).
I was also thinking of including some basic textual instructions (or at least a link?) for the winpcap installation, what do you think?

@remgodow
Copy link
Contributor Author

@imsnif Sure, feel free to adapt it for project needs. We could add an information for Windows users that WinPcap or NpCap in WinPcap compatible mode is required for bandwhich to work, with a link to NpCap, as WinPcap is depracated. An extra for windows users would be a package for chocolatey/scoop package managers.

@remgodow remgodow marked this pull request as ready for review September 10, 2020 17:31
@remgodow
Copy link
Contributor Author

Scoop looks easy, its uses a simple json with project description and links to binaries as a package manifest. To add it to the repository you make a pull to scoop repo. Here is an example for ccache

@imsnif
Copy link
Owner

imsnif commented Sep 11, 2020

Hey @remgodow, there seems to be a git hitch that I'm not sure I have the permissions to fix here. Could you please merge main into here? I did this locally but am not able to push. I think it's just getting everything from main except the actions themselves.

@imsnif
Copy link
Owner

imsnif commented Sep 11, 2020

Also, @remgodow - as a side note, I would very much like to add you as an author in Cargo.toml - since you made a very significant contributions. If you'd like that, please add yourself to the bottom of the authors with your email address (if you'd like to include it).

Remigiusz Godowicz added 2 commits September 11, 2020 18:08
# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	build.rs
#	src/os/linux.rs
#	src/os/lsof.rs
#	src/os/lsof_utils.rs
#	src/os/shared.rs
@remgodow
Copy link
Contributor Author

The issue with pushing to the branch is caused by release action - right now, it creates /refs/heads/action tags. Therefore it is neccesary to delete release and tag on the fork page to allow pushes. I've added you as collaborator to my fork, I hope that it will allow you to test actions and remove test releases.

@imsnif
Copy link
Owner

imsnif commented Sep 13, 2020

I played around with this a little. Github actions look cool, but tbh - while I do see the benefit of releasing additional executables, this is too much of a time sink for me right now. Seeing as this isn't blocking anything (to my knowledge) at the moment, I'm going to leave this open and get back to it if the need becomes more pressing. Thanks again for the work on this and the windows port!

@remgodow
Copy link
Contributor Author

remgodow commented Sep 13, 2020

@imsnif I have adopted release action to current release versioning, and added musl target. Would you need anything else?
You can see the effects here: https://github.com/remgodow/bandwhich/releases/tag/0.19.7

@imsnif
Copy link
Owner

imsnif commented Sep 14, 2020

This looks great! Thanks :)

A few things:

  1. When unpacking the tar.gz file, I get all the folders of the release in the repository (target/x86_64-unknown-linux-musl/release), to fix this I usually tar from the same folder as the original file, but I think there's also a tar flag for this.
  2. Would it be possible to make the release a manual step (I think I saw this is possible in github actions) rather than a regex on the commit?
  3. How would we get a visual indication for these in the PR? Like we get the pass/fail now with travis and a link to the log? Or will it just happen once we merge?

…d on github, tar executable without directories
@remgodow
Copy link
Contributor Author

  1. Fixed by using working-directory parameter
  2. I changed release action so that: it no longer creates release by itself, instead it triggers on "release: created" event - when a release is created using Github, then it builds binaries and uploads them to created release. https://github.com/remgodow/bandwhich/releases/tag/0.20.0
  3. When you configure branch protection, it automatically detects github actions and you can select actions which will be required to succeed to allow merging. Checks indicator looks like travis one - Eventresize remgodow/bandwhich#1 (I guess it does not run because actions are not in main, but you can see the UI).

@imsnif
Copy link
Owner

imsnif commented Sep 15, 2020

This looks really great. I don't have a lot of time to devote to it right now, but I plan on releasing a new version in the next few days, so will definitely merge this and use it for the release beforehand.

@imsnif imsnif merged commit 58187b4 into imsnif:main Sep 20, 2020
@imsnif
Copy link
Owner

imsnif commented Sep 20, 2020

So, unfortunately working with github acations is a no-go for me at the moment. I merged this and tried to work with them, but they fail consistently on macos because of some sort of race condition.

I realize that the fault here is in the tests themselves and that a proper fix would be to find the race condition and fix it there, but after spending some time on it I can see that this might not be trivial. At the moment, the travis tests do what we want them to (they sometimes also fail here as well, but much more rarely and almost always run successfully when re-run). So, for our current needs I think they are enough.

Thanks very much for your work on this. I realize you put a lot of effort here. We gave it a good go, but I guess at least in our case, travis is a better option. The actions are still in main but I disabled github actions on this repository so the output will not confuse those wanting to contribute. Maybe in the future I'll give them another go.

@remgodow
Copy link
Contributor Author

Sure thing, the benefit of nigtly and ci actions over travis was having the artifacts, but its not a deal breaker when it comes to developent builds, hovever there is also a release actions which works on all platforms and is triggered manually by creating a release (as it builds and uploads the artifacts, without testing). Therefore I would suggest deleting nigtly.yaml and ci.yaml, keeping the release.yaml. This way you could benefit from artifacts for releases, while keeping travis for CI/pull requests.

@remgodow
Copy link
Contributor Author

By the way, if there is anything I can help with, let me know. I may have some time next week, and I've seen that the windows port caused issues for other systems through the dependencies.

@imsnif
Copy link
Owner

imsnif commented Sep 20, 2020

By the way, if there is anything I can help with, let me know. I may have some time next week, and I've seen that the windows port caused issues for other systems through the dependencies.

Uff, yeah - it was a hot mess but I think I took care of it here: #190 :)

If you'd like something else to work on, this has resurfaced recently: #150
Also, I've been thinking of adding a flag that would let you manually specify a DNS server instead of autodetecting it.
Only if you feel like it of course. Your contributions are very much welcome.

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.

2 participants