search

DOI-USGS / usgscsm

This repository stores USGS Community Sensor Model (CSM) camera models
Other
26 stars 33 forks source link

Update usgscsm install location #467

Closed chkim-usgs closed 7 months ago

chkim-usgs commented 8 months ago

Update usgscsm install location to be ${CMAKE_INSTALL_LIBDIR}/csmplugins which should be equivalent to $CONDA_PREFIX/lib/csmplugins so it is easier to find these custom plugins. ISIS's IsisPreferences:CSMDirectory value will be updated to include this path at the very top as default path once this PR is merged in.

Licensing

This project is mostly composed of free and unencumbered software released into the public domain, and we are unlikely to accept contributions that are not also released into the public domain. Somewhere near the top of each file should have these words:

This work is free and unencumbered software released into the public domain. In jurisdictions that recognize copyright laws, the author or authors of this software dedicate any and all copyright interest in the software to the public domain.

Kelvinrr commented 8 months ago

We should get the PR to fix the build issues in before merging this.

acpaquette commented 8 months ago

Build issues here are linked to needing a new ALE release with changes from

https://github.com/DOI-USGS/ale/pull/575

Probably a 0.10.0 ALE release, potentially a 1.0.0

Kelvinrr commented 8 months ago

@oleg-alexandrov does this make sense as an install location ($INSTALL_PREFIX/lib/csmplugins)? I looked around and pinged Trent and it seems there isn't a lot of precedence in terms of where they are typically installed... Are there other examples of CSM plugins we can use or should we make up our own?

oleg-alexandrov commented 8 months ago

I have no preference. I don't think there's also any other plugins we need to be concerned about.

I ran into a problem with the usgscsm plugin. ASP links to libusgscsm.so as to a regular library, because it needs a lot of logic that is not accessible via the CSM interface. But then, when ISIS loads this plugin from a different location, it appears that the library is in memory twice, and I get a crash in a destructor somewhere. The error is: malloc_consolidate(): invalid chunk size

The solution seems to be to have the library just in one place.

My best guess is that what you folks are planning is to modify the usgscsm conda package and have it install its libraries in your csmplugins dir, and then that location will be set in the ISIS preferences. When you do that, and new ISIS and usgscsm versions are released, I will have to update ASP accordingly to tell it where to look up these libraries, instead of lib/, where they are now.

Kelvinrr commented 8 months ago

I have no preference. I don't think there's also any other plugins we need to be concerned about.

I ran into a problem with the usgscsm plugin. ASP links to libusgscsm.so as to a regular library, because it needs a lot of logic that is not accessible via the CSM interface. But then, when ISIS loads this plugin from a different location, it appears that the library is in memory twice, and I get a crash in a destructor somewhere. The error is: malloc_consolidate(): invalid chunk size

The solution seems to be to have the library just in one place.

My best guess is that what you folks are planning is to modify the usgscsm conda package and have it install its libraries in your csmplugins dir, and then that location will be set in the ISIS preferences. When you do that, and new ISIS and usgscsm versions are released, I will have to update ASP accordingly to tell it where to look up these libraries, instead of lib/, where they are now.

Correct, we expect this to be an API breaking change so any versions after the next release will have an updated install location, the goal is to only have it in one location that we dont expect to change anytime soon. /lib makes it difficult to load multiple plugins unless you know the exact lib you're loading (Jay ran into an issue where he loaded everything in his lib folder). And the one that ISIS defaults to right now has the version hardcoded into the path which I dont think is ideal as it would require updating the path every version bump. Lastly, putting it in a path that isn't tied to usgs seems like a better location if someone wants to dump other plugins there.

Overall, I think this change can be sustained long term unlike the previous path.

chkim-usgs commented 7 months ago

~Separate PR to update install location and fix build issues has been merged: https://github.com/DOI-USGS/usgscsm/pull/470 As such, will close this PR.~ Apologies, there was a mix up in closing PRs, updated this PR appropriately and still open for review.

chkim-usgs commented 7 months ago

@Kelvinrr yes, although it looks like there isn't a CHANGELOG.md file for usgscsm? I can always create one though and we can start adding to the changelog from here on out.

Kelvinrr commented 7 months ago

@chkim-usgs Sorry, must have not noticed. We should keep a changelog. I can merge this and if you dont mind, can you add one in a separate PR?