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

Minor build system tweaks #293

Merged
merged 7 commits into from
Sep 8, 2023

Conversation

sanjayankur31
Copy link
Contributor

Hello,

We made a few tweaks to the build project files to adhere to the Fedora packaging guidelines while including MorphIO in Fedora. They:

  • use the GNUInstallDirs so that files are installed in the standard locations
  • install the python binding shared object to the right place
  • version the shared object

We're carrying these patches in Fedora at the moment, but we'd like to stay as close to upstream as possible. Could the dev team please take a look to see if these could be merged here upstream?

Initially, use 0.0.0 but ideally, the soversion should follow semver to
indicate ABI changes etc.

Using this will also create the necessary symlinks

References:
- https://cmake.org/cmake/help/latest/prop_tgt/SOVERSION.html
- https://semver.org/
@mgeplf
Copy link
Contributor

mgeplf commented Apr 23, 2021

Thanks, we will have to check that this works on our side.

I don't think we'll be able to use the chosen SOVERSION, but you're right that it needs to be set, and we need to make a clear decision about that - appreciate it.

@sanjayankur31
Copy link
Contributor Author

Thanks, we will have to check that this works on our side.

I don't think we'll be able to use the chosen SOVERSION, but you're right that it needs to be set, and we need to make a clear decision about that - appreciate it.

Thanks very much.

I've set the SOVERSION to 0.0.0 to sort of signify that one hasn't been set (a default), so that whatever the dev team here decide will be greater than the current version.

@mgeplf
Copy link
Contributor

mgeplf commented Jul 12, 2021

Sorry, I"m way behind and haven't had a chance to do this; it hasn't been forgotten though!

@mgeplf
Copy link
Contributor

mgeplf commented Sep 17, 2021

I re-iterate the above comment - looking at this now.

matz-e
matz-e previously approved these changes Feb 24, 2022
Copy link
Member

@matz-e matz-e left a comment

Choose a reason for hiding this comment

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

LGTM

@mgeplf mgeplf dismissed matz-e’s stale review February 24, 2022 09:57

don't want this merged yet; we'll be checking it - but that's for the review!

Copy link
Member

@matz-e matz-e left a comment

Choose a reason for hiding this comment

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

Building and loading py-morphio in Spack is fine, as is using morphio as a dependency for TouchDetector and running a test circuit. So from my side, good to go in.

@sanjayankur31
Copy link
Contributor Author

Hello, any progress here please? (If this gets merged, I won't have to carry and re-port these patches to new versions to update the Fedora packages, so that'll make life a little easier for us downstream).

@mgeplf
Copy link
Contributor

mgeplf commented Aug 16, 2023

Hello, any progress here please? (If this gets merged, I won't have to carry and re-port these patches to new versions to update the Fedora packages, so that'll make life a little easier for us downstream).

I'm so sorry, I genuinely don't want to make maintainers life harder, so we'll try and get this in for the next version we release.

What sort of timeline are you on?

@sanjayankur31
Copy link
Contributor Author

Hello, any progress here please? (If this gets merged, I won't have to carry and re-port these patches to new versions to update the Fedora packages, so that'll make life a little easier for us downstream).

I'm so sorry, I genuinely don't want to make maintainers life harder, so we'll try and get this in for the next version we release.

What sort of timeline are you on?

We're not on an urgent time line here. If this can be incorporated into one of the upcoming releases so we don't have to backport it ourselves downstream, that'll be great.

Thanks for the help.

@mgeplf mgeplf merged commit e75d9f4 into BlueBrain:master Sep 8, 2023
12 checks passed
@mgeplf
Copy link
Contributor

mgeplf commented Sep 8, 2023

@sanjayankur31 thanks a lot for the change; I think we're good to go with it.

@sanjayankur31 sanjayankur31 deleted the feat/build-system-tweaks branch September 8, 2023 09:36
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