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

Build shared libraries, add run-export #44

Merged
merged 13 commits into from
Apr 28, 2023
Merged

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented Apr 23, 2023

Towards #38

Closes #41

@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.

@h-vetinari h-vetinari force-pushed the shared branch 4 times, most recently from 194a15b to a0fa312 Compare April 27, 2023 09:27
@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@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.

@h-vetinari h-vetinari marked this pull request as ready for review April 27, 2023 11:57
@h-vetinari
Copy link
Member Author

@lidavidm @wjones127
This should be ready to go; it's now according to conda-forge default standards (only shared libs), with a run-export, and with a bunch of clean-ups. PTAL :)

Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Well done! The tests look quite thorough. Hopefully I can get the share libraries fixed upstream so we don't need that patch.

* @h-vetinari @lidavidm
Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like @lidavidm will need to grant you write access to the repo if you want to be added as a codeowner.

Copy link
Member Author

Choose a reason for hiding this comment

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

That warning is just GitHub being silly. Merging the PR is all that's necessary to join as a maintainer.

Copy link
Contributor

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thank you!

@@ -15,6 +15,8 @@ cmake .. ^
-DCMAKE_CXX_STANDARD=17 ^
-DCMAKE_PREFIX_PATH=%CONDA_PREFIX% ^
-DCMAKE_INSTALL_PREFIX=%LIBRARY_PREFIX% ^
-DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON ^
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this may need to be fixed upstream (presumably dllimport/dllexport annotations are missing)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but those are really invasive and that's why many projects don't have them 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

more practically, google never fixed the protobuf compiler to be able to insert them properly so I guess it's not possible to fix anyways. (at least the last time I took a swing at that.)

@lidavidm lidavidm merged commit 2a84b86 into conda-forge:main Apr 28, 2023
@h-vetinari h-vetinari deleted the shared branch April 28, 2023 02:02
@h-vetinari
Copy link
Member Author

Thanks for merging! :)

@h-vetinari
Copy link
Member Author

Thanks for merging! :)

... though I wish you hadn't squashed everything. 🤷 Makes it harder to do archeology etc. on the feedstock.

@lidavidm
Copy link
Contributor

Ah sorry, it appears to default to squash for me and usually I just go with that. I'll try to keep that in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants