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

Jump to bottom

Refactor multi output and add omniorbpy #53

Merged
merged 15 commits into from
Jul 8, 2024
Merged

Refactor multi output and add omniorbpy #53

merged 15 commits into from
Jul 8, 2024

Conversation

beenje
Copy link
Contributor

@beenje beenje commented May 9, 2024 •
edited
Loading

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Refactored the multi-output as current solution was quite fragile as pointed out in #46

Add omniorbpy as new output as discussed in conda-forge/gepetto-viewer-corba-feedstock#22

This allows to:

  • release both omniorb and omniorbpy at the same time
  • make omniorbpy depends on the exact version of omniorb
  • build omniorbpy on windows (it needs to be built from under omniorb/src/lib)

When merged, the https://github.com/conda-forge/omniorbpy-feedstock needs to be archived.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

@beenje
Copy link
Contributor Author

beenje commented May 9, 2024

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 2 commits May 9, 2024 15:47
@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@beenje
Copy link
Contributor Author

beenje commented May 9, 2024

@conda-forge-admin, please rerender

@beenje beenje marked this pull request as draft May 9, 2024 20:00
@beenje
Add pkg-config to build tools
3b6f4ff 
Try to fix openssl detection on Linux
@beenje
Remove patch-configure.diff
a3cdb53 
Patch was overwritten by autoconf - doesn't seem needed
@beenje
Add patch to find openssl root
337b73c 
openssl 3.3.0 doesn't include prefix in the pkgconfig file.

pkg-config finds openssl but "pkg-config --variable=prefix openssl" returns an empty string.
OPEN_SSL_ROOT is empty and it prevents ssl transport to be compiled (even if openssl is reported as found).

Patch used to return the prefix when that value is empty.
@beenje
Add patch to find openssl root to omniorbpy
2c80e21 
Same patch as for omniorb
@beenje
Add omniorb-libs to omniorbpy host dependency
9c42659 
Solve WARNING (omniorbpy,lib/python3.10/site-packages/_omnipy.cpython-310-x86_64-linux-gnu.so.4.3): .. but ['local/linux-64::omniorb-libs==4.3.2=h40b4993_3'] not in reqs/run, (i.e. it is overlinking) (likely) or a missing dependency (less likely)
@beenje beenje marked this pull request as ready for review May 10, 2024 16:50
@beenje
Copy link
Contributor Author

beenje commented May 10, 2024

@lockhart I refactored the recipe multi output and included omniORBpy.

I had issues compiling the ssl transport with the latest openssl. configure reported it found openssl but compilation was skipped. Issue was that pkg-config --variable=prefix openssl reports an empty string and that was used for OPEN_SSL_ROOT. I applied a small patch to use the prefix instead (in both omniorb and omniorbpy) and opened an issue in conda-forge/openssl-feedstock#155

omniorbpy build still raises some warning about omniorb-libs not being in reqs/run. It is in host.
In run, we have {{ pin_subpackage('omniorb', exact=True) }}, which itself also as in run {{ pin_subpackage('omniorb-libs', exact=True) }}. I guess I could add it as well?

@h-vetinari , as you contributed to this recipe before, I'd appreciate some review. If you have time of course.

@lockhart
Copy link

lockhart commented May 10, 2024 via email

@github-actions GitHub Actions
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/omniorb-feedstock/actions/runs/9035882141.

h-vetinari
h-vetinari reviewed May 10, 2024
View reviewed changes
Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

I just skimmed the diff for a bit, but it looks like a very good effort. Well done!

(PS. I haven't really contributed here except helping various boost migrations)

@h-vetinari
Copy link
Member

h-vetinari commented May 10, 2024 •
edited
Loading

omniorbpy build still raises some warning about omniorb-libs not being in reqs/run. It is in host.
In run, we have {{ pin_subpackage('omniorb', exact=True) }}, which itself also as in run {{ pin_subpackage('omniorb-libs', exact=True) }}. I guess I could add it as well?

I don't know the exact reason why, but run-exports don't get applied in the recipe where the output (that should have the run-export) in question is being built. So you should add a run-dep for {{ pin_subpackage('omniorb-libs', exact=True) }}

@beenje
Add omniorb-libs as explicit run-dep for omniorbpy
739da6b 
run-exports don't get applied in the recipe where the output in question is being built
@beenje
Store built artifacts on azure
a5dd584
@beenje
Copy link
Contributor Author

beenje commented May 17, 2024

@conda-forge-admin, please rerender

@beenje
Copy link
Contributor Author

beenje commented May 17, 2024

Pinging @conda-forge/omniorbpy explicitly as it impacts your feedstock.

I updated the feedstock to store artifacts on azure. To test, you can download them here

@beenje
Copy link
Contributor Author

beenje commented Jul 5, 2024

@lockhart if you are happy with this PR, please approve and I will merge

lockhart
lockhart approved these changes Jul 5, 2024
View reviewed changes
Copy link

@lockhart lockhart left a comment

Choose a reason for hiding this comment

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

Combining omniorb and omniorbpy into a single package solves the problem of conda not recognizing unique micro-releases and having omniorbpy built against the previous version of omniorb. These changes have been tested on my Sonoma M3 laptop against a significant production app and all features passed.

@lockhart
Copy link

lockhart commented Jul 5, 2024 via email

@beenje beenje merged commit 1627676 into conda-forge:main Jul 8, 2024
32 checks passed
This was referenced Jul 8, 2024
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h-vetinari h-vetinari h-vetinari left review comments

@lockhart lockhart lockhart approved these changes

@carlodri carlodri Awaiting requested review from carlodri carlodri is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@beenje @lockhart @h-vetinari