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.

Sign up for GitHub

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 GTK3 support for linux x86-64 to maximize compatibility #1795

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
+34 −13
Draft

Build GTK3 support for linux x86-64 to maximize compatibility #1795

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0467081
Build GTK3 support for linux x86-64 to maximize compatibility
jonahgraham Feb 3, 2025
4109c4a
Temporary edit to force natives to be rebuilt
jonahgraham Feb 4, 2025
cfa6a6e
Simplify runOnNativeBuildAgent and remove duplicated code
jonahgraham Feb 5, 2025
9d1226f
Explicitly request bash for scripts using bash
jonahgraham Feb 5, 2025
3b4c79c
Remove the clunky DEST_PLATFORM and simplify
jonahgraham Feb 5, 2025
d4618d4
Revert "Temporary edit to force natives to be rebuilt"
jonahgraham Feb 5, 2025
fd1ea9c
Add missing break on switch
jonahgraham Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 34 additions & 9 deletions Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

def runOnNativeBuildAgent(String platform, Closure body) {
def final nativeBuildStageName = 'Build SWT-native binaries'
if (platform == 'gtk.linux.x86_64') {
if (platform == 'gtk4.linux.x86_64') {
podTemplate(inheritFrom: 'basic' /* inherit general configuration */, containers: [
containerTemplate(name: 'swtbuild', image: 'eclipse/platformreleng-debian-swtnativebuild:12',
resourceRequestCpu:'1000m', resourceRequestMemory:'512Mi',
Expand All @@ -26,6 +26,15 @@ def runOnNativeBuildAgent(String platform, Closure body) {
]) {
node(POD_LABEL) { stage(nativeBuildStageName) { container('swtbuild') { body() } } }
}
} else if (platform == 'gtk.linux.x86_64') {
podTemplate(inheritFrom: 'basic' /* inherit general configuration */, containers: [
containerTemplate(name: 'swtbuild', image: 'eclipse/platformreleng-debian-swtgtk3nativebuild:10',
resourceRequestCpu:'1000m', resourceRequestMemory:'512Mi',
resourceLimitCpu:'2000m', resourceLimitMemory:'4096Mi',
alwaysPullImage: true, command: 'cat', ttyEnabled: true)
]) {
node(POD_LABEL) { stage(nativeBuildStageName) { container('swtbuild') { body() } } }
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having two blocks one for each config with a lot of shared configuration, we could only have one block and the different images defined before:

	def dockerImage = null;
	switch (platform) {
		case 'gtk.linux.x86_64': dockerImage = 'eclipse/platformreleng-debian-swtgtk3nativebuild:10';
		case 'gtk4.linux.x86_64': dockerImage = 'eclipse/platformreleng-debian-swtnativebuild:12';
	}
	if (dockerImage != null) {
		podTemplate(inheritFrom: 'basic' /* inherit general configuration */, containers: [
			containerTemplate(name: 'swtbuild', image: dockerImage,
				resourceRequestCpu:'1000m', resourceRequestMemory:'512Mi',
				resourceLimitCpu:'2000m', resourceLimitMemory:'4096Mi',
				alwaysPullImage: true, command: 'cat', ttyEnabled: true)
		]) {
			node(POD_LABEL) { stage(nativeBuildStageName) { container('swtbuild') { body() } } }
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above has a small mistake, I needed to add break to the case statements. See latest commit fd1ea9c The result is the https://ci.eclipse.org/releng/job/eclipse.platform.swt/job/PR-1795/7/ built both gtk3 and gtk4 with the gtk4 Debian image.

Once build finishes (which may be tomorrow for me now) I will look if that worked.

The good news is that my manual testing showed the problem! This change is still in draft state because I also want to bring in equinox/features/org.eclipse.equinox.executable.feature/library/gtk/check_dependencies.sh that is part of eclipse-equinox/equinox#834 to make sure the compiled libs match expectation at build time.

} else {
// See the Definition of the RelEng Jenkins instance in
// https://github.com/eclipse-cbi/jiro/tree/master/instances/eclipse.platform.releng
Expand Down Expand Up @@ -199,6 +208,7 @@ pipeline {
'gtk.linux.ppc64le',\
'gtk.linux.riscv64',\
'gtk.linux.x86_64',\
'gtk4.linux.x86_64',\
'win32.win32.aarch64',\
'win32.win32.x86_64'
}
Expand All @@ -220,22 +230,29 @@ pipeline {
runOnNativeBuildAgent("${PLATFORM}") {
cleanWs() // Workspace is not cleaned up by default, so we do it explicitly
echo "OS: ${os}, ARCH: ${arch}"
unstash "swt.binaries.sources.${ws}"
unstash "swt.binaries.sources.${ws == 'gtk4' ? 'gtk' : ws }"
dir('jdk.resources') {
unstash "jdk.resources.${os}.${arch}"
}
withEnv(['MODEL=' + arch, "OUTPUT_DIR=${WORKSPACE}/libs", "SWT_JAVA_HOME=${WORKSPACE}/jdk.resources"]) {
if (isUnix()){
sh '''
Copy link
Member

Choose a reason for hiding this comment

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

As the script below that runs on the native machine now uses bash specific commands ([[) and bash seems not to be the default shell on all native machines, we have to add a shebang.

Suggested change
sh '''
sh '''#!/bin/bash -x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 that makes sense, the block further down that relies on bash runs on the main build machine. I will include the shebang on it too.

mkdir libs
if [ "${PLATFORM}" = "gtk.linux.aarch64" ]; then
if [[ ${PLATFORM} == gtk.linux.* ]]; then
sh build.sh -gtk3 install
elif [ "${PLATFORM}" = "gtk.linux.ppc64le" ]; then
sh build.sh -gtk3 install
elif [ "${PLATFORM}" = "gtk.linux.riscv64" ]; then
sh build.sh -gtk3 install
else
elif [[ ${PLATFORM} == gtk4.linux.* ]]; then
# We build both 3 + 4, but we only keep libswt-pi4-gtk
# (see 'Collect and sign binaries' stage)
#
# We build both to help catch build errors as this
# build runs against more modern gcc/libs and helps
# with verification
sh build.sh install
elif [[ ${PLATFORM} == cocoa.macosx.* ]]; then
sh build.sh install
else
echo "Unexpected build platform ${PLATFORM}"
exit 1
fi
ls -1R libs
'''
Expand All @@ -259,9 +276,13 @@ pipeline {
dir("libs/${PLATFORM}") {
unstash "swt.binaries.${PLATFORM}"
sh '''
DEST_PLATFORM=${PLATFORM}
if [[ ${PLATFORM} == cocoa.macosx.* ]]; then
binariesExtension='jnilib'
signerUrl='https://cbi.eclipse.org/macos/codesign/sign'
elif [[ ${PLATFORM} == gtk4.linux.x86_64 ]]; then
binariesExtension='so'
DEST_PLATFORM=gtk.linux.x86_64
Copy link
Member

Choose a reason for hiding this comment

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

We could probably save this block when using a slightly adjusted pattern in the next block(given the change below is applied):
elif [[ ${PLATFORM} == gtk*.linux.* ]]; then

But it's probably simpler to understand if we are explicit there. On the other hand we could still be explicit and save this block if we just combine both block and 'or' the arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the updated commit when I submit, but I reworked this section to make it cleaner and achieved something I wanted to do originally.

elif [[ ${PLATFORM} == gtk.linux.* ]]; then
binariesExtension='so'
elif [[ ${PLATFORM} == win32.win32.* ]]; then
Expand All @@ -278,7 +299,11 @@ pipeline {
done
fi
fi
cp *.$binariesExtension "${WORKSPACE}/eclipse.platform.swt/binaries/org.eclipse.swt.${PLATFORM}/"
if [[ ${PLATFORM} == gtk4.linux.x86_64 ]]; then
cp libswt-pi4-gtk*.so "${WORKSPACE}/eclipse.platform.swt/binaries/org.eclipse.swt.${DEST_PLATFORM}/"
else
cp *.$binariesExtension "${WORKSPACE}/eclipse.platform.swt/binaries/org.eclipse.swt.${DEST_PLATFORM}/"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we implement this without the extra DEST_PLATFORM variable and therefore slightly simpler?

Suggested change
if [[ ${PLATFORM} == gtk4.linux.x86_64 ]]; then
cp libswt-pi4-gtk*.so "${WORKSPACE}/eclipse.platform.swt/binaries/org.eclipse.swt.${DEST_PLATFORM}/"
else
cp *.$binariesExtension "${WORKSPACE}/eclipse.platform.swt/binaries/org.eclipse.swt.${DEST_PLATFORM}/"
fi
if [[ ${PLATFORM} == gtk4.linux.x86_64 ]]; then
cp libswt-pi4-gtk*.so "${WORKSPACE}/eclipse.platform.swt/binaries/org.eclipse.swt.gtk.linux.x86_64/"
else
cp *.$binariesExtension "${WORKSPACE}/eclipse.platform.swt/binaries/org.eclipse.swt.${PLATFORM}/"
fi

'''
}
}
Expand Down
2 changes: 2 additions & 0 deletions bundles/org.eclipse.swt/Eclipse SWT/common/library/swt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,5 @@ void throwOutOfMemory(JNIEnv *env) {
(*env)->ThrowNew(env, clazz, "");
}
}

// Fake edit to force natives to be rebuilt as recommended in https://github.com/eclipse-platform/eclipse.platform.swt/pull/1795#issuecomment-2632885057
Loading