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 zig-build build style, river 0.1.0, rundird 0.2.0 #29288

Merged
merged 4 commits into from
Nov 13, 2021

Conversation

ifreund
Copy link
Contributor

@ifreund ifreund commented Mar 7, 2021

General

Have the results of the proposed changes been tested?

  • I use the packages affected by the proposed changes on a regular basis and confirm this PR works for me
  • I generally don't use the affected packages but briefly tested this PR

Closes #33060

@ericonr ericonr added the new-package This PR adds a new package label Mar 8, 2021
@ericonr
Copy link
Member

ericonr commented Mar 8, 2021

It would be nice if it could do cross :)

@travankor
Copy link
Contributor

travankor commented Mar 11, 2021

It would be nice if it could do cross :)

blocked on #25004 ?

hopefully, cross is not a blocker for initial packaging

@ericonr
Copy link
Member

ericonr commented Mar 11, 2021

I don't think so. That PR is about cross compiling the compiler, not projects using zig...

And the template here isn't trying to pass any targets or anything, so the fix might even be a simple one.

@travankor
Copy link
Contributor

travankor commented Mar 11, 2021

oh woops, sorry

This is what happens when you only look at the PR title.

edit: maybe zig build -target ${XBPS_CROSS_TRIPLET} -Drelease-safe ?

common/build-style/zig-build.sh Outdated Show resolved Hide resolved
common/build-style/zig-build.sh Outdated Show resolved Hide resolved
common/build-style/zig-build.sh Outdated Show resolved Hide resolved
common/build-style/zig-build.sh Outdated Show resolved Hide resolved
srcpkgs/zig/template Show resolved Hide resolved
srcpkgs/rundird/template Show resolved Hide resolved
@ifreund ifreund force-pushed the rundird branch 3 times, most recently from 83dda10 to fb852ac Compare June 21, 2021 21:08
@ifreund
Copy link
Contributor Author

ifreund commented Jun 21, 2021

2 of the patches have not yet been upstreamed, I'll be doing that shortly. As such, I'd like to hold off on merging this until they are upstreamed.

@ifreund ifreund marked this pull request as draft June 21, 2021 21:09
@ifreund ifreund force-pushed the rundird branch 2 times, most recently from 706f9dc to 9302c24 Compare June 21, 2021 21:12
@ifreund ifreund marked this pull request as ready for review June 26, 2021 11:07
@ifreund ifreund changed the title New package: rundird-0.1.0 New package: rundird-0.1.1 Jun 26, 2021
@ifreund ifreund changed the title New package: rundird-0.1.1 New package: rundird-0.1.1, Add zig-build build style Jun 26, 2021
srcpkgs/rundird/files/rundird/run Outdated Show resolved Hide resolved
common/build-style/zig-build.sh Outdated Show resolved Hide resolved
@ericonr
Copy link
Member

ericonr commented Jul 25, 2021

diff --git a/common/build-style/zig-build.sh b/common/build-style/zig-build.sh
index 69dea4ca55..8c11822b70 100644
--- a/common/build-style/zig-build.sh
+++ b/common/build-style/zig-build.sh
@@ -1,7 +1,5 @@
 do_build() {
-	local zig_abi
-	local zig_target
-	local zig_cpu
+	local zig_abi zig_target zig_cpu zig_arch
 
 	case $XBPS_TARGET_LIBC in
 		glibc) zig_abi="gnu";;
@@ -11,16 +9,18 @@ do_build() {
 
 	case $XBPS_TARGET_MACHINE in
 		aarch64*|i686*|x86_64*)
-			zig_target="${XBPS_TARGET_MACHINE%-musl}-linux-${zig_abi}" zig_cpu="baseline";;
+			zig_arch="${XBPS_TARGET_MACHINE%-musl}" zig_cpu="baseline";;
 		armv6l*) zig_target="arm-linux-${zig_abi}" zig_cpu="generic+v6";;
 		armv7l*) zig_target="arm-linux-${zig_abi}" zig_cpu="generic+v7a";;
 		ppc64le*) zig_target="powerpc64le-linux-${zig_abi}" zig_cpu="baseline";;
 		ppc64*) zig_target="powerpc64-linux-${zig_abi}" zig_cpu="baseline";;
 		ppcle*) zig_target="powerpcle-linux-${zig_abi}" zig_cpu="baseline";;
 		ppc*) zig_target="powerpc-linux-${zig_abi}" zig_cpu="baseline";;
-		*) broken="TODO: support more target machines for the zig build style";;
+		*) msg_error "TODO: support more target machines for the zig build style\n";;
 	esac
 
+	zig_target="${zig_arch}-linux-${zig_abi}"
+
 	# Inform zig of the required libc include paths.
 	cat > xbps_zig_libc.txt <<-EOF
 		include_dir=${XBPS_CROSS_BASE}/usr/include

Do for all archs what I did for x86; and broken is checked during template parsing time, not during do_build. This is "better" but still breaks builds; you need something in an environment file to set broken so XBPS doesn't error out. To avoid duplicating the logic, could create XBPS_ZIG_ABI ... and use those instead of local variables.

@ifreund
Copy link
Contributor Author

ifreund commented Jul 27, 2021

Thanks for the review @ericonr! I've gone ahead and done this "properly" by defining zig target and cpu variables in the build-profiles and cross-profiles. I think this is a lot better as it makes it much easier to keep the zig target cpu features in sync with the ones used for C as the definitions are in the same file. In fact, I caught a couple of minor mistakes I had made in the previous approach this way.

This should be ready to merge from my point of view, assuming CI passes.

@ifreund ifreund changed the title New package: rundird-0.1.1, Add zig-build build style New package: rundird-0.2.0, Add zig-build build style Jul 27, 2021
Comment on lines +6 to +13
if [ "$CROSS_BUILD" ]; then
zig_target="${XBPS_CROSS_ZIG_TARGET}"
zig_cpu="${XBPS_CROSS_ZIG_CPU}"
else
zig_target="${XBPS_ZIG_TARGET}"
zig_cpu="${XBPS_ZIG_CPU}"
fi
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about an environment/build-style/ file that sets archs for the supported archs? This way you don't have to detect if these are set before do_build in order to trigger broken (or maybe it's the best way, since it avoids duplicating the information?).

Overall looks really good, just want to be sure of the best way of signaling when zig is unsupported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, that seems like it might be a good idea in general but I don't think we need that for zig. Zig is theoretically able to target all of xbps's targets and I've included exhaustive support for xbps's targets in this PR.

However, support for some of the less common architectures is of course not as good and attempting to actually compile code for them may fail due to issues in the compiler and/or std. This of course depends on the exact code being compiled as well though, especially when the issues are due to stdlib deficiencies which are often easy to add temporary downstream workarounds for as the language matures.

@ifreund
Copy link
Contributor Author

ifreund commented Aug 18, 2021

Just rebased onto master and fixed a typo in a comment that I just noticed.

@ifreund
Copy link
Contributor Author

ifreund commented Sep 7, 2021

Zig 0.8.1 has been released with the the patches I landed to support sysroot-based cross compilation dynamically linking system libraries. Therefore we no longer need to patch zig to enable the zig-build build style and I've rebased this PR onto #32867.

@ifreund ifreund changed the title New package: rundird-0.2.0, Add zig-build build style Add zig-build build style, river 0.1.0, rundird 0.2.0 Nov 3, 2021
@ifreund ifreund mentioned this pull request Nov 3, 2021
@ifreund ifreund force-pushed the rundird branch 3 times, most recently from ab555a4 to fbe6360 Compare November 8, 2021 15:58
@subsetpark
Copy link

@ifreund @ericonr are there still outstanding issues for this PR? I'd love to get River into my xbps managed packages...

crt_dir=${XBPS_CROSS_BASE}/usr/lib
msvc_lib_dir=
kernel32_lib_dir=
gcc_dir=
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to query gcc_dir from GCC itself? Would it be worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

according to the output of zig libc, gcc_dir is only needed when targeting haiku:

# The directory that contains `crtbeginS.o` and `crtendS.o`
# Only needed when targeting Haiku.
gcc_dir=

common/cross-profiles/i686.sh Outdated Show resolved Hide resolved
srcpkgs/zig/template Show resolved Hide resolved
@ifreund ifreund force-pushed the rundird branch 2 times, most recently from 920fa2d to 2602db1 Compare November 11, 2021 11:25
@ifreund
Copy link
Contributor Author

ifreund commented Nov 11, 2021

@ericonr Thanks for the review! I've enabled sse2 on i686 and split the zig package fix into a separate commit, this should now be good to go afaik.

@ericonr
Copy link
Member

ericonr commented Nov 13, 2021

Ok, tested both packages, seem to be working just fine. All that's missing is installing the example file and possibly a vdoc README.md in rundird, what do you think?

This commit backports one bug fix from the zig master branch which is
critical for our use-case of building software for binary distribution.
We call this "zig-build" instead of just "zig" as this build-style
relies on usage of the zig build system. In the future, other build
systems such as meson may support zig code. Furthermore, the zig
build system may be used to build C/C++ code as well, not just zig.
@ifreund
Copy link
Contributor Author

ifreund commented Nov 13, 2021

Ok, tested both packages, seem to be working just fine. All that's missing is installing the example file and possibly a vdoc README.md in rundird, what do you think?

Excellent! Good call on including the example river init and rundird README.md, especially for rundird as I forgot that I never wrote a man page for that. Not that there's much that would go in a man page, the only important bit of end-user documentation is the PAM configuration.

@ericonr ericonr merged commit fd08510 into void-linux:master Nov 13, 2021
@ifreund ifreund deleted the rundird branch November 13, 2021 14:58
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new-package This PR adds a new package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package Request: river
4 participants