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: allow building without scion-ci #4449

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

mlegner
Copy link
Contributor

@mlegner mlegner commented Nov 24, 2023

My motivation for this is an error on ARM64 machines when building scion-ci:

(13:40:17) ERROR: /home/ubuntu/.cache/bazel/_bazel_ubuntu/d590279010957ac1dca373a0e19d0c3c/external/io_bazel_rules_go/BUILD.bazel:86:17: While resolving toolchains for target @io_bazel_rules_go//:cgo_context_data: No matching toolchains found for types @bazel_tools//tools/cpp:toolchain_type.

However, it may be interesting irrespective of this to just build the main binaries without the CI tools.

Maybe the naming could be improved, let me know what you think.


This change is Reviewable

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Thanks!

For context: the error occurs because building some of the tools requires cgo. Specifically, this is braccept which uses the cgo-based gopacket/afpacket library to write raw packets.
As I see it, the braccept utility is solely built used for the router_multi test, which is triggered from within bazel, and does not need the output of the scion-ci target. As an alternative approach, I think it could make sense to just drop braccept from the scion-ci target.

If we decide to go for the approach with different make targets, I think it could make sense to call the non-ci variant just something plain like build, and the variant with the CI tools something like build-dev.
Or, something like dist-bin could make sense for the no-CI variant; I was planning to add a make target dist-deb for building debian packages (see WIP #4448), so this could also fit.

Reviewable status: 0 of 1 files reviewed, all discussions resolved

Copy link
Contributor

@jiceatscion jiceatscion left a comment

Choose a reason for hiding this comment

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

gopacket is also used by brload now.

Reviewable status: 0 of 1 files reviewed, all discussions resolved

@mlegner
Copy link
Contributor Author

mlegner commented Nov 24, 2023

If we decide to go for the approach with different make targets, I think it could make sense to call the non-ci variant just something plain like build, and the variant with the CI tools something like build-dev.
Or, something like dist-bin could make sense for the no-CI variant; I was planning to add a make target dist-deb for building debian packages (see WIP #4448), so this could also fit.

All of these sound fine to me, no strong preference either way. I'm happy to update this PR, but feel free to just close it and include this in a separate PR.

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Ok. Let's just add the make target that you propose then.
My preference for the name would be something like dist, dist-bin, or build (and build-dev for the "ci" variant) -- nothing with "bazel".
Please mark the target as .PHONY.

Reviewable status: 0 of 1 files reviewed, all discussions resolved

@mlegner mlegner force-pushed the build/makefile-no-ci branch from f687837 to e079858 Compare November 27, 2023 10:56
@mlegner
Copy link
Contributor Author

mlegner commented Nov 27, 2023

@matzf How about this version with build and build-dev and build-dev as the default target?

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

💯

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mlegner)


doc/dev/setup.rst line 75 at r2 (raw file):

      make

  .. hint:: This installs tools for tests in addition to the main SCION services (e.g., `end2end`);

Nit: I think "installs" could be mistaken here, just "build" or "compile" might be better.

@mlegner mlegner force-pushed the build/makefile-no-ci branch from e079858 to 1938a94 Compare November 27, 2023 12:46
Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mlegner)

@matzf matzf enabled auto-merge (squash) November 27, 2023 15:29
The default target remains unchanged but is renamed to `build-dev`.
The target `build` now only builds the main SCION services.
auto-merge was automatically disabled November 27, 2023 16:12

Head branch was pushed to by a user without write access

@mlegner mlegner force-pushed the build/makefile-no-ci branch from e6a8a93 to 247e90c Compare November 27, 2023 16:12
@matzf matzf merged commit 237794a into scionproto:master Nov 27, 2023
4 checks passed
@mlegner mlegner deleted the build/makefile-no-ci branch November 27, 2023 17:51
juagargi pushed a commit to netsec-ethz/scion that referenced this pull request Mar 8, 2024
The default target remains unchanged but is renamed to `build-dev`.
The target `build` now only builds the main SCION services.
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