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 FreeBSD process helper to facilitate FreeBSD builds #531

Merged
merged 3 commits into from
May 22, 2024

Conversation

zi0r
Copy link
Contributor

@zi0r zi0r commented May 13, 2024

These patches are already part of the FreeBSD port for pueue. Including them here will make port updates a little easier.

You can view existing applied patches here.

To completely fix the build, we also need to remove procfs/libproc from the Cargo files [while building under Freebsd]. I wasn't sure on the best way to achieve this, so I am simply noting it here.

Checklist

Please make sure the PR adheres to this project's standards:

  • I included a new entry to the CHANGELOG.md.
  • I checked cargo clippy and cargo fmt. The CI will fail otherwise anyway.

@Nukesor
Copy link
Owner

Nukesor commented May 14, 2024

Check the pueue/Cargo.toml at the very bottom.

Change this:

# Test specific dev-dependencies
[target.'cfg(any(target_os = "linux", target_os = "freebsd"))'.dependencies]
whoami = "1"
procfs = { version = "0.16", default-features = false }

To this:

# Test specific Linux + BSD specific dev-dependencies
[target.'cfg(any(target_os = "linux", target_os = "freebsd"))'.dependencies]
whoami = "1"

# Test specific Linux dev-dependencies
[target.'cfg(target_os = "linux")'.dependencies]
procfs = { version = "0.16", default-features = false }

In pueue_lib/Cargo.toml, the following needs to be changed:

# Unix
[target.'cfg(unix)'.dependencies]
libproc = "0.14.6"
whoami = "1"

# Linux only
[target.'cfg(target_os = "linux")'.dependencies]
procfs = { version = "0.16", default-features = false }

Becomes

# Unix
[target.'cfg(unix)'.dependencies]
whoami = "1"

[target.'cfg(any(target_os = "linux", target_os = "macos"))'.dependencies]
libproc = "0.14.6"

# Linux only
[target.'cfg(target_os = "linux")'.dependencies]
procfs = { version = "0.16", default-features = false }

So. If Pueue is to have proper support for FreeBSD, I want this to be properly tested.

Please:

  • Add tests
  • Make sure the test suite runs on FreeBsd.
    -> Unit tests should succeed, integration tests should work if possible, otherwise just don't build them as can be seen in pueue/tests/tests.rs. From what I understand, the test worked at some point for FreeBSD and actually used procfs, which is why I'm a bit confused that it's now no longer allowed.
  • Investigate how to run the test suite for FreeBSD in the CI pipeline. The best way to achieve this is to fork the repo and test the pipelines on your own repo.

@zi0r
Copy link
Contributor Author

zi0r commented May 14, 2024

Thanks for the tip! I've added an additional commit to break out the procfs support further. While FreeBSD does still have a way to mount procfs, it isn't enabled by default. This adds another step for users and is wonderful to be able to bypass it whenever we have another means.

Unfortunately, I don't believe I have time to dedicate to revamping the tests suite or CI Pipeline for FreeBSD support. However, I can tell you that the commits in this PR (or a close variation of them) have been in use in the FreeBSD ports tree since pueue v3.0.0, when it was first added on Dec 25 2022. You can view the history here.

Lastly, there is one additional patch that we include (adopted from alpine linux):

I'm happy to add this to my PR, if desired.

@Nukesor
Copy link
Owner

Nukesor commented May 20, 2024

Heyo, sorry for the late response. Had somewhat of a busy week.

When I read that patch correctly, it adds #![allow(bindings_with_variant_name)] to two files, which has been done by me last year at 2023-04-29 in commit 892213e. Those changes have been long released.

The freebsd discussion that ported the fix:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=272467

@Nukesor
Copy link
Owner

Nukesor commented May 20, 2024

It's super fascinating to see how pueue is packaged :D
I'm ususally not involved into any packaging work at all, but there's quite a lot of stuff going on over there.

In the future, I would be more than happy to release a new patch whenever something breaks due to changes in the build environment/toolchain. Just give me a ping :)

Copy link
Owner

@Nukesor Nukesor left a comment

Choose a reason for hiding this comment

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

Changes look good :)
I'll wait for the CI to pass and push a patch release soon afterwards!

Edit: Small format lint 😁

Copy link

codecov bot commented May 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.30%. Comparing base (63aba54) to head (cffb17c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #531      +/-   ##
==========================================
+ Coverage   80.28%   80.30%   +0.01%     
==========================================
  Files          77       77              
  Lines        5666     5666              
==========================================
+ Hits         4549     4550       +1     
+ Misses       1117     1116       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Test Results

  3 files   22 suites   2m 53s ⏱️
148 tests 148 ✅ 0 💤 0 ❌
316 runs  316 ✅ 0 💤 0 ❌

Results for commit cffb17c.

@zi0r
Copy link
Contributor Author

zi0r commented May 20, 2024

It's super fascinating to see how pueue is packaged :D I'm ususally not involved into any packaging work at all, but there's quite a lot of stuff going on over there.

In the future, I would be more than happy to release a new patch whenever something breaks due to changes in the build environment/toolchain. Just give me a ping :)

This sounds amazing and will certainly make the updating process on the FreeBSD ports tree side of things much easier! Thanks!

@Nukesor Nukesor merged commit cffb17c into Nukesor:main May 22, 2024
12 of 19 checks passed
@Nukesor
Copy link
Owner

Nukesor commented May 22, 2024

Fixed that lint for you :)

Thanks for the contribution! I'll release a patch soon.

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

2 participants