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 system #21

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Build system #21

merged 1 commit into from
Nov 8, 2024

Conversation

daniel-noland
Copy link
Collaborator

@daniel-noland daniel-noland commented Sep 23, 2024

basically the whole project up to this point. looking to squash and start clean after this lands

@daniel-noland daniel-noland force-pushed the real-readme branch 2 times, most recently from ebeb8d4 to 7cfb0fd Compare September 24, 2024 00:57
@daniel-noland daniel-noland marked this pull request as ready for review September 24, 2024 00:59
.cargo/config.toml Outdated Show resolved Hide resolved
dpdk-sys/gen-sysroot/Dockerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
dpdk/build.rs Outdated Show resolved Hide resolved
dpdk-sys/build.rs Outdated Show resolved Hide resolved
dpdk-sys/build.rs Outdated Show resolved Hide resolved
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

A few fixes are necessary, as per the discussion above, so I'm turning my review status into “Request changes”.

scratch/build.rs Outdated Show resolved Hide resolved
@daniel-noland daniel-noland force-pushed the real-readme branch 23 times, most recently from 8f23c5e to a8246c9 Compare September 26, 2024 18:30
@daniel-noland daniel-noland force-pushed the real-readme branch 8 times, most recently from 6f09590 to 68047cd Compare November 7, 2024 23:28
@daniel-noland daniel-noland changed the title draft: use build.rs to remove dnoland paths Build system Nov 7, 2024
@daniel-noland
Copy link
Collaborator Author

A few fixes are necessary, as per the discussion above, so I'm turning my review status into “Request changes”.

I think I have addressed everything you discussed before. Let me know what you think

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good, awesome work! 🚀

The PR is too big to really dig into the details. I pulled the changes and validated that I can still build and pass the tests with both glibc/musl (Note that I get a few warnings for the tests). I'd normally complain that this needs to be split up into logical commits, but I understand the plan is to wipe the Git history so far anyway.

I've got a few nits below, but we can address them as follow-ups. I think we should go ahead and merge this PR, we can do some more polishing afterwards.

Comment on lines 1 to 8
# The primary point of this workflow is to ensure that the developer experience is good.
# We take a very vanilla ubuntu image, install all necessary dependencies via "normal" means,
# and then run the build and test steps as described in the README.md file.

# The artifacts produced by these builds are not intended to be used for anything other than
# ensuring that the developer experience is good.

# Production artifacts are produced in a sterile environment (in another CI workflow).
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems copy-pasted from dev.yml but I don't think it applies to this workflow.

Cargo.toml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@@ -0,0 +1,90 @@
# Hedgehog Dataplane
Copy link
Member

Choose a reason for hiding this comment

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

Next step: We need a cool logo :)

This comment hasn't been addressed 😛

README.md Outdated Show resolved Hide resolved
@qmonnet
Copy link
Member

qmonnet commented Nov 8, 2024

Note: I have a follow-up PR based on yours, and some of the sterile.yml / push checks are failing (https://github.com/githedgehog/dataplane/actions/runs/11743968718/job/32717988208?pr=62)

@qmonnet
Copy link
Member

qmonnet commented Nov 8, 2024

checks are failing

ERROR: invalid tag "ghcr.io/githedgehog/dataplane:2024-11-08.pr/license.x86_64-unknown-linux-gnu.debug.f940553fe3b427a4b71fcf5df3837b8f17ed63ca": invalid reference format

I think this is because I used a / in my branch name for the PR (I used pr/license). We need to sanitize the branch name before using it in a container tag, here in the justfile:

_branch := `git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "sterile"`

@qmonnet
Copy link
Member

qmonnet commented Nov 8, 2024

I've got a fix for this branch name thing:

diff --git a/justfile b/justfile
index d87adb36099a..2c9be0c97c4c 100644
--- a/justfile
+++ b/justfile
@@ -66,7 +66,7 @@ _commit := `git rev-parse HEAD 2>/dev/null || echo "sterile"`
 # The git branch we are currnetly on
 # We allow this command to fail in the sterile environment because git is not available there
 
-_branch := `git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "sterile"`
+_branch := `(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "sterile") | tr -c '[:alnum:]\n' '-'`
 
 # The git tree state (clean or dirty)
 # We allow this command to fail in the sterile environment because git is not available there

I'll do a follow-up PR after your branch is merged.

@daniel-noland
Copy link
Collaborator Author

I've got a fix for this branch name thing:

diff --git a/justfile b/justfile
index d87adb36099a..2c9be0c97c4c 100644
--- a/justfile
+++ b/justfile
@@ -66,7 +66,7 @@ _commit := `git rev-parse HEAD 2>/dev/null || echo "sterile"`
 # The git branch we are currnetly on
 # We allow this command to fail in the sterile environment because git is not available there
 
-_branch := `git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "sterile"`
+_branch := `(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "sterile") | tr -c '[:alnum:]\n' '-'`
 
 # The git tree state (clean or dirty)
 # We allow this command to fail in the sterile environment because git is not available there

I'll do a follow-up PR after your branch is merged.

nice

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