Remove all existing native libraries before copying new compiled #1100
Conversation
|
@HannesWell : I'm not sure why that was done in the way it was done, I guess we still don't build all native bits on SWT native build, which would be sad. I guess some parts like webkit / edge support libraries might be deleted now and not rebuilt => therefore DRAFT PR. |
iloveeclipse
force-pushed
the
issue_1095
branch
from
50e796a
to
f739f8e
Compare
There was a problem hiding this comment.
@HannesWell : I'm not sure why that was done in the way it was done, I guess we still don't build all native bits on SWT native build, which would be sad.
I guess some parts like webkit / edge support libraries might be deleted now and not rebuilt => therefore DRAFT PR.
In general the structure was similar to the ant-task based build but I wanted to make deleting the natives more specific to not just delete all native files in the fragments. As you mentioned we cannot build all native binaries. The only one I see currently is the WebView2Loader.dll which is provided by Microsoft. I think this is a suitable reference:
https://learn.microsoft.com/en-us/microsoft-edge/webview2/concepts/distribution#files-to-ship-with-the-app
Jenkinsfile
Outdated
| @@ -263,6 +263,7 @@ pipeline { | |||
| done | |||
| fi | |||
| fi | |||
| rm "${WORKSPACE}/eclipse.platform.swt/binaries/org.eclipse.swt.${PLATFORM}/*swt*.$binariesExtension" | |||
There was a problem hiding this comment.
The place where the deletion is supposed to happen is
eclipse.platform.swt/Jenkinsfile
Lines 153 to 155 in d06d1a1
For the reasons you have described this does not delete the old binaries if the minor or major version is changed manually (i.e. before the pipeline starts).
But instead of deleting the binaries here the pattern in the linked rm calls should be adjusted to cover that scenario too.
So it could either be just changed to
rm -f binaries/org.eclipse.swt.gtk.*/libswt-*.so
rm -f binaries/org.eclipse.swt.win32.*/swt-*.dll
rm -f binaries/org.eclipse.swt.cocoa.*/libswt-*.jnilib
or more specific (not sure if this the right syntax I just searched it together) to expect a version like suffix in the end.
With regex I would write \d+r\d+
rm -f binaries/org.eclipse.swt.gtk.*/libswt-*-+([0-9])r+([0-9]).so
rm -f binaries/org.eclipse.swt.win32.*/swt-*-+([0-9])r+([0-9]).dll
rm -f binaries/org.eclipse.swt.cocoa.*/libswt-*-+([0-9])r+([0-9]).jnilib
HannesWell
force-pushed
the
issue_1095
branch
3 times, most recently
from
eea222c
to
744c816
Compare
There was a problem hiding this comment.
@iloveeclipse I have now applied the suggested changes and pushed it to your branch.
As I'm not a Linux pro, I cannot tell if there is a better way to express the suggested regex style pattern for bash. Please let me know if you do.
I have re-started the build of this PR enforcing native-lib build so we can see if this change works as expected. In my MSYS environment under Windows it worked as expected.
Once this is merged we have to enforce a native re-build on the master in order to delete the old natives or they are deleted manually as part of this PR.
I have no strong preference for either way. Deleting them manually would be more work, but would save us new revisions and thus one or two MB of LFS space.
| @@ -150,9 +150,10 @@ pipeline { | |||
| 'comma_ver='+swtVersions['comma_ver'], "new_comma_ver=${swtVersions['maj_ver']},${swtVersions['min_ver']},${swtVersions['new_rev']},0" ]) { | |||
| sh ''' | |||
There was a problem hiding this comment.
+(pattern‐list) syntax is a bashism (extglob). This won't work in POSIX sh.
We need to switch to bash here and enable extglob:
-sh '''
+sh '''#!/bin/bash -ex
+ shopt -s extglobThere was a problem hiding this comment.
@sratz : what about
swtNativeLibName='swt-.*-[0-9]+r[0-9]+'
ls -d binaries/org.eclipse.swt.gtk.*/* | grep -E ${swtNativeLibName}.so | xargs rm -vf
ls -d binaries/org.eclipse.swt.win32.*/* | grep -E ${swtNativeLibName}.dll | xargs rm -vf
ls -d binaries/org.eclipse.swt.cocoa.*/* | grep -E ${swtNativeLibName}.jnilib | xargs rm -vf
There was a problem hiding this comment.
No, please don't. Never parse ls output. https://mywiki.wooledge.org/ParsingLs
There was a problem hiding this comment.
thanks for that hint! It's really not simple to understand all these shell scripts in detail 😅
With all this special setup I think it is best, to just go the simple way and use:
rm binaries/org.eclipse.swt.gtk.*/libswt-*.so
rm binaries/org.eclipse.swt.win32.*/swt-*.dll
rm binaries/org.eclipse.swt.cocoa.*/libswt-*.jnilib
With the current schema this deletes exactly the binaries we build in the SWT pipeline.
All native libraries in the fragments that should not be deleted at this point simply must not start with swt/libswt, which is the case for the only current candidate WebView2Loader.dll.
If 'make_common.mak' is changed manually (e.g. during new release-cycle preparation), the code in 'Check if SWT-binaries build is needed' stage, that is supposed to delete native libraries, doesn't delete anything as it relies on getSWTVersions() that reads the (already changed) versions. Adjust the file-pattern to delete every *swt* library file stored in git instead, regardless of its exact version. Fixes eclipse-platform#1095 Co-authored-by: Hannes Wellmann <wellmann.hannes1@gmx.net>
|
The current list of changes looks good (see below). |
|
I'm fine with that. |
Then lets apply this. :) |
If make_common.mak changes, code in 'Check if SWT-binaries build is needed' stage, that is supposed to delete native libraries, simply doesn't delete anything as it relies on getSWTVersions() that reads (changed) versions.
Instead, try to delete every library file stored in git before copying freshly compiled ones during 'Collect and sign binaries' stage.
Fixes #1095