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

[cmake]: Fix usage after install when built with BUILD_SHARED_LIBS=ON #2016

Merged

Conversation

DownerCase
Copy link
Contributor

Description

Fixes the build such that the installed result is usable when eCAL is built with BUILD_SHARED_LIBS=ON.

  • tcp_pubsub submodule is now always built statically as it is a strictly internal dependency.
  • ecal-utils may now be static or shared depending on BUILD_SHARED_LIBS
    • It was required to be installed as it was used by other static/shared libraries
    • There was no particular reason for it to always be static so now it aligns with the other non-fixed libraries
  • EcalParser, QEcalParser, CustomQt, and ThreadingUtils are now always static because they were implementation details for applications and application plugins and were not required to be installed.

Related issues

Fixes #2014

Cherry-pick to

ecal-utils is used by other exported libraries (eg: ecalhdf5) which may
be static or shared so there is no need to constrain ecal-utils, it
should be consistent with other libraries
ThreadingUtils is only used internally by applications so it can be static and
not exported/installed
Both are only used by executables and aren't exported so they can be
forced static
It is only used in applications and monitor plugins so it can be static
@FlorianReimold FlorianReimold added the cherry-pick-to-NONE Don't cherry-pick these changes label Feb 12, 2025
Copy link
Member

@FlorianReimold FlorianReimold left a comment

Choose a reason for hiding this comment

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

I am not happy with hiding the Windows signing issues. It's not a mandatory action and we can merge with the sign job failing, but I would rather have it notify me of failures than report OK despite not working.

Edit: or is it just about (not) signing from forks? I am struggeling with the script.

@DownerCase
Copy link
Contributor Author

DownerCase commented Feb 12, 2025

I am not happy with hiding the Windows signing issues. It's not a mandatory action and we can merge with the sign job failing, but I would rather have it notify me of failures than report OK despite not working.

Edit: or is it just about (not) signing from forks? I am struggeling with the script.

Yeah, the change should only affect forks.

The problem is my runs don't have access to your secrets so the JENKINS_* environment variables don't get set, which causes the "Sign the installer on Eclipse CI" step to be skipped (because otherwise it would fail from missing auth).
The signing step sets IS_DOWNLOAD_AVAILABLE to true and populates ASSET_NAME environment variables.

The other option is to do something with GitHub environments and pull_request_target: https://github.com/orgs/community/discussions/50161#discussioncomment-5824158

Alternatively, if you don't like the deployment spam you'd get with that approach, could do something directly with pull_request_target and change your repo settings such that checks always require approval before running (like what should happen for first-time contributors).


From what I can tell, the workflow will correctly fail if something other than the secrets being unavailable goes wrong, and when the signing is skipped only the "unsigned-setup" artifact will be uploaded.

Seems fine to me, but if you want me to drop the change just say and I'll open an issue to capture this.

(Also, sorry for shoehorning the change in, it wasn't really necessary and obviously could have been done separately...)

@FlorianReimold FlorianReimold merged commit c18fd66 into eclipse-ecal:master Feb 13, 2025
14 checks passed
@DownerCase DownerCase deleted the fix-build-shared-install branch February 13, 2025 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants