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: add option EMBED_PROJ_DATA_PATH #4207

Merged
merged 5 commits into from
Aug 12, 2024

Conversation

WangWeiLin-MV
Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV commented Jul 24, 2024

Add an option EMBED_PROJ_DATA_PATH to allow PROJ_DATA_PATH not to be hardcoded. Defaults to ON, does not change existing behavior.

There are many ways to get the path, and the existing code allows macros not to be defined, for example src/filemanager.cpp#L1390-L1394.

There are many situations where you have to concern about hard coding path which might be fragile and breaks easily, or just want file path independent in build time. So, the option is added.

@WangWeiLin-MV WangWeiLin-MV marked this pull request as ready for review July 24, 2024 09:01
@kbevers
Copy link
Member

kbevers commented Jul 24, 2024

Could you please explain why this is desirable to you?

@rouault
Copy link
Member

rouault commented Jul 24, 2024

Could you please explain why this is desirable to you?

Besides original reporter's own needs, I see this could be of value for example for conda-forge builds, where PROJ is relocated in a arbitrary location on the user machine, and thus hardcoding PROJ_DATA makes little sense.

@kbevers
Copy link
Member

kbevers commented Jul 24, 2024

I'm sure there's a benefit, I just think it is good form to explain why we should consider a pull request.

This needs adding to the docs as well, by the way. Under CMake configure options in https://github.com/OSGeo/PROJ/blob/master/docs/source/install.rst

@jjimenezshaw
Copy link
Contributor

Could you please explain why this is desirable to you?

Besides original reporter's own needs, I see this could be of value for example for conda-forge builds, where PROJ is relocated in a arbitrary location on the user machine, and thus hardcoding PROJ_DATA makes little sense.

That would be useful for me too. When you do not know where your proj is going to be located, it makes no sense. Even worse, it could be that there is an older version installed there, and just takes the wrong proj.db

@@ -414,6 +414,12 @@ All cached entries can be viewed using ``cmake -LAH`` from a build directory.
Path to an existing directory used to cache :file:`proj.db` to speed-up
subsequent builds without modifications to source SQL files.

.. option:: EMBED_PROJ_DATA_PATH

.. versionadded:: 9.4
Copy link
Contributor

@jjimenezshaw jjimenezshaw Jul 25, 2024

Choose a reason for hiding this comment

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

9.4.1 is already released. Should it be 9.5, the next release?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think the description below this needs a bit more work. When I'm reading this it only clear to my that this hardcodes the PROJ_DATA_PATH into the binaries because I've read the discussion in this PR. A short explanation of why you might want to turn this off would be very helpful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I have made the changes, please review this PR and if you have any questions, please let me know.

@jjimenezshaw
Copy link
Contributor

Could you please explain why this is desirable to you?

Besides original reporter's own needs, I see this could be of value for example for conda-forge builds, where PROJ is relocated in a arbitrary location on the user machine, and thus hardcoding PROJ_DATA makes little sense.

That would be useful for me too. When you do not know where your proj is going to be located, it makes no sense. Even worse, it could be that there is an older version installed there, and just takes the wrong proj.db

Another use case that happened to me (just in case it is happening to anybody else): "it works in my (developer's) computer"

I am using PROJ in an application that can be located anywhere in the filesystem. PROJ_DATA had to be set accordingly at the beginning (or right before) of the process. However it was working (in my computer) without setting it... because magically it was using the proj.db from my compilation folder, not the distribution folder. Even if the CI computer, because it was first compiling PROJ and then the application. We noticed it later when we tested manually somewhere else that never compiled PROJ. We changed the CI workflow to build and run in different instances.

@rouault
Copy link
Member

rouault commented Jul 25, 2024

Actually, I stand corrected regarding conda-forge. Conda has magic to patch strings !

Hard-coded path (with placeholder padding) in the "raw" package downloaded from the forge:

$ strings /home/even/miniconda3/pkgs/proj-9.4.1-hb784bbd_0/lib/libproj.so.25.9.4.1 | grep share/proj
../share/proj
/home/conda/feedstock_root/build_artifacts/proj_1718272424126/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/share/proj

vs substituted paths in the package installed in the conda env:

$ strings /home/even/miniconda3/envs/gdal_3.9.1/lib/libproj.so.25.9.4.1 | grep share/proj
../share/proj
/home/even/miniconda3/envs/gdal_3.9.1/share/proj

@jjimenezshaw
Copy link
Contributor

Conda has magic to patch strings !

I am a bit scared. How is it doing that?

@rouault
Copy link
Member

rouault commented Jul 25, 2024

I am a bit scared. How is it doing that?

https://docs.conda.io/projects/conda-build/en/stable/resources/make-relocatable.html

Co-authored-by: Javier Jimenez Shaw <j1@jimenezshaw.com>
@rouault rouault merged commit bddac14 into OSGeo:master Aug 12, 2024
22 of 23 checks passed
@rouault rouault added this to the 9.5.0 milestone Aug 12, 2024
@WangWeiLin-MV WangWeiLin-MV deleted the cmake/option-embed-path branch August 13, 2024 02:00
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.

4 participants