geotrans/3.8 by samuel-emrys · Pull Request #6512 · conan-io/conan-center-index

samuel-emrys · 2021-07-26T15:24:25Z

Specify library name and version: geotrans/3.8

I saw that there was an outstanding library request for it (#1779). I wanted to learn about conan packaging and this looked like a good candidate, since I also wanted to check the library out. I'm not the author of this library.

One of the hook tests is currently failing. I've raised #6511 for discussion of this issue and an appropriate way forward. I figured it deserved it's own issue as it raises questions about the assumptions of the "no imports" test more generally than just this package.

closes #1779

I've read the guidelines for contributing.
I've followed the PEP8 style guides for Python code in the recipes.
I've used the latest Conan client version.
I've tried at least one configuration locally with the
conan-center hook activated.

* Add CMakeLists.txt to build geotrans library * Add CCI package configuration * Add functional test package using geotrans library provided sample code

* Build both libMSPdtcc and libMSPCoordinateConversionService to remain consistent with the upstream package. This is a change from the previous state in which both of these libraries were bundled into one. * Remove extraneous comments from test_package conanfile * Remove extraneous print statements from test_package example.cpp

* Add component definitions to package_info. This will allow cmake utilities like find_package to search for and find the package and its component libraries, which will enable linking against these individually for more granular control.

* No longer a shared library by default * Update homepage metadata * Update URL metadata

CLAassistant · 2021-07-26T15:24:38Z

All committers have signed the CLA.

recipes/geotrans/all/CMakeLists.txt

recipes/geotrans/all/conandata.yml

recipes/geotrans/all/conanfile.py

ruilvo · 2021-07-26T16:20:35Z

For the record: closes #1779

recipes/geotrans/all/CMakeLists.txt

recipes/geotrans/all/conanfile.py

recipes/geotrans/all/test_package/CMakeLists.txt

recipes/geotrans/all/test_package/conanfile.py

recipes/geotrans/all/conanfile.py

recipes/geotrans/all/test_package/conanfile.py

madebr · 2021-07-26T20:51:46Z

Thanks for your first submission.
It looks very good, although it lacks support for Macos and MSVC.

Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com> Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

* Fix CMake installation process * Refactor CMakeLists to be operated from the root source directory rather than source_subfolder. This removes the requirements to use the shutil package from within conanfile.py * Transition to usage of find_package for Threads library. This will allow thread library discovery to be performed in an OS agnostic way. * Test consumer package sets the required MPCCS_DATA environment variable to the data folder within the conan package folder rather than copying it to the consumers working directory. This is for CCI CI runner testing efficiency purposes, elimintating the need for a copy.

recipes/geotrans/all/CMakeLists.txt

This will ideally make it easier for the user to understand the requirements of them to correctly configure the MPCCS_DATA environment variable prior to attempting to use the geotrans library.

recipes/geotrans/all/conanfile.py

* Delete fPIC option when shared library is being built * Remove superfluous shutil import

Co-authored-by: Rui Oliveira <ruimail24@gmail.com>

* Remove superfluous fPIC management from config_options function * Remove superfluous C library requirements to remove cpp flags

@madebr

* Link MSPdtcc to MSPCoordinateConversionService to satisfy linkage requirements on macOS. This rectifies CI failures. * Remove cmake_find_package(_multi) assignments from conanfile.py at the advice of @madebr

* Move responsibility for setting MSPCCS_DATA environment variable from consumer to package * Remove usage output from build method * Remove explicit assignment of resdir value Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>

* Remove version number of library from CMakeLists.txt * Remove version number of library from conanfile.py * Move definition of CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS from conanfile to CMakeLists.txt * Add commentary on requisite environment variables to be set to consumer conanfile.py * Re-add call to tools.environment_append to ensure that MPCCS_DATA environment variable is set when executing the example binary

jgsogo · 2021-09-28T14:46:46Z

I've written to the organization to see if it is possible to get a link with versioned sources... let's see if they answer in a couple of days.

samuel-emrys · 2021-10-01T09:14:19Z

I've written to the organization to see if it is possible to get a link with versioned sources... let's see if they answer in a couple of days.

To follow this up, we received a response from NGA today. My understanding of the response is that because they're not interested in supporting legacy versions, they are not going to provide a versioned download link at this stage. They have indicated that they will consider the wider community need for a versioned download link and may revise this position in the future.

My read of this is that they may be convinced to go to the effort of providing a download link if geotrans sees sufficient popularity on conan, if there are sufficient complaints about packages breaking when they release 3.9 (scheduled within the next 9 months), or perhaps just part of their 3.9 release process.

In the meantime, it appears as though finding an alternate way to mirror the source code would be the best way forward. @jgsogo do you agree with my summary/have anything to add?

jgsogo · 2021-10-14T10:12:19Z

Yes, same message.

I'm proposing a PR here to store these sources (conan-io/cci-sources-backup#2). We cannot use regular Git because the file is too big, and (free) LFS storage/bandwith in GitHub is quite limited. We can configure LFS to use Artifactory, so we have plenty of room for these files.

However, my main concern now is how to get the link from the repo. It requires some translation from

version https://git-lfs.github.com/spec/v1
oid sha256:baa72d3b1ae12f237a8ad30f2deb3fed2b80feb759528ea0a72b4b42cb77c565
size 159618792

to the actual URL:

https://c3i.jfrog.io/artifactory/cci-sources-backup/objects/ba/a7/baa72d3b1ae12f237a8ad30f2deb3fed2b80feb759528ea0a72b4b42cb77c565

Random thoughts:

given that we only need to get the URL once, probably that tranlations is not a problem at all
is the Git repo still useful to track the sources?
should we just use a generic Artifactory repo and forget about Git proxy?

jgsogo

Please, use sources from the mirror in our Artifactory.

recipes/geotrans/all/conandata.yml

samuel-emrys · 2021-10-14T22:12:35Z

Yes, same message.

I'm proposing a PR here to store these sources (conan-io/cci-sources-backup#2). We cannot use regular Git because the file is too big, and (free) LFS storage/bandwith in GitHub is quite limited. We can configure LFS to use Artifactory, so we have plenty of room for these files.

However, my main concern now is how to get the link from the repo. It requires some translation from
version https://git-lfs.github.com/spec/v1
oid sha256:baa72d3b1ae12f237a8ad30f2deb3fed2b80feb759528ea0a72b4b42cb77c565
size 159618792
to the actual URL:
https://c3i.jfrog.io/artifactory/cci-sources-backup/objects/ba/a7/baa72d3b1ae12f237a8ad30f2deb3fed2b80feb759528ea0a72b4b42cb77c565
Random thoughts:
* given that we only need to get the URL once, probably that _tranlations_ is not a problem at all

* is the Git repo still useful to track the sources?

* should we just use a generic Artifactory repo and forget about Git proxy?

I realise that we're moving forward with a generic Artifactory repo in lieu of a LFS solution for the moment, but to provide some more discussion about why the Artifactory solution is preferred (at least from my perspective). There are practical considerations to using LFS that are messy. Namely, what is the desired workflow? Normally, this is:

User forks repo
User makes changes
User submits PR

There seem like two alternatives here:

Users are able to configure their forked repo to use JFrog Artifactory to back LFS. This would be required as users would be unable to push large file changes to their fork otherwise. This would mean they would be unable to make a PR as the changes can't be propagated to their github remote. I can't imagine this is ideal.
Only users who can engage directly with the cci-sources-backup repo can make changes. This circumvents the need to submit a PR from a fork, but limits the number of people capable of making changes. How can the community at large contribute here? I guess the solution would be issues with instructions that require action from JFrog staff.

I guess the Artifactory solution here is really the same as solution (2) above, as I would imagine only JFrog staff will have the ability to upload new sources (?). Regardless, I think the Artifactory solution simplifies things a bit as solution (1) becomes infeasible. The URL is simpler too.

madebr · 2021-10-14T22:15:32Z

Let's KISS this. git is a handy tool, but we don't have to push it everywhere.

* Update geotrans 3.8 source URL to use JFrog Artifactory. This resolves a build reproducibility issue caused by the fact that NGA don't host a versioned mirror of the source files. The JFrog mirror provided is now versioned and will be able to be downloaded even after the release of geotrans 3.9 Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>

conan-center-bot · 2021-10-14T23:03:48Z

All green in build 15 (77a972e897d66b530c28e25f7c199c2f443c7f68):

geotrans/3.8@:
All packages built successfully! (All logs)

jgsogo

🚀

Sorry for the delay and all the back and forth regarding the sources... but finally the package will be there. Thanks a lot for your patience!

SSE4 · 2021-10-15T11:38:52Z

Let's KISS this. git is a handy tool, but we don't have to push it everywhere.

KISS is exactly the reason, that's why we have chosen plain artifactory over artifactory + git lfs

SSE4 · 2021-10-15T16:41:10Z

finally!

samuel-emrys added 4 commits July 25, 2021 12:12

Add geotrans package to close conan-io#1779

7125ecf

* Add CMakeLists.txt to build geotrans library * Add CCI package configuration * Add functional test package using geotrans library provided sample code

Add component definitions to package_info

6638006

* Add component definitions to package_info. This will allow cmake utilities like find_package to search for and find the package and its component libraries, which will enable linking against these individually for more granular control.

Rectify errors identified by cci hooks

b11f52c

* No longer a shared library by default * Update homepage metadata * Update URL metadata

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Jul 26, 2021

samuel-emrys mentioned this pull request Jul 26, 2021

[question] "NO IMPORTS" policy on test packages #6511

Closed

ruilvo suggested changes Jul 26, 2021

View reviewed changes

recipes/geotrans/all/CMakeLists.txt Outdated Show resolved Hide resolved

recipes/geotrans/all/CMakeLists.txt Outdated Show resolved Hide resolved

recipes/geotrans/all/conandata.yml Outdated Show resolved Hide resolved

recipes/geotrans/all/conanfile.py Outdated Show resolved Hide resolved

SpaceIm reviewed Jul 26, 2021

View reviewed changes

recipes/geotrans/all/CMakeLists.txt Outdated Show resolved Hide resolved

madebr reviewed Jul 26, 2021

View reviewed changes

samuel-emrys and others added 2 commits July 30, 2021 23:30

Apply suggestions from code review

cd1c510

Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com> Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

This comment has been minimized.

Sign in to view

Resolve issues with suggested changes in review

f3ceff0

SSE4 reviewed Jul 30, 2021

View reviewed changes

recipes/geotrans/all/CMakeLists.txt Outdated Show resolved Hide resolved

Add output to inform the user about geotrans MPCCS_DATA envvar

3201118

This will ideally make it easier for the user to understand the requirements of them to correctly configure the MPCCS_DATA environment variable prior to attempting to use the geotrans library.

This comment has been minimized.

Sign in to view

ruilvo suggested changes Jul 30, 2021

View reviewed changes

recipes/geotrans/all/conanfile.py Show resolved Hide resolved

Rectify fPIC management as per KB-H007 to address CI issues

688b378

* Delete fPIC option when shared library is being built * Remove superfluous shutil import

This comment has been minimized.

Sign in to view

samuel-emrys and others added 2 commits July 31, 2021 01:33

Update recipes/geotrans/all/conanfile.py

3425370

Co-authored-by: Rui Oliveira <ruimail24@gmail.com>

Remove fPIC management from config_options function

aab06b1

* Remove superfluous fPIC management from config_options function * Remove superfluous C library requirements to remove cpp flags

This comment has been minimized.

Sign in to view

Link dtcc library to ccs and remove cmake_find_package values

5f0fa06

* Link MSPdtcc to MSPCoordinateConversionService to satisfy linkage requirements on macOS. This rectifies CI failures. * Remove cmake_find_package(_multi) assignments from conanfile.py at the advice of @madebr

This comment has been minimized.

Sign in to view

Remove -Wno-deprecated compilation flag for windows compatability

32dd18f

This comment has been minimized.

Sign in to view

conan-center-bot requested review from czoido and memsharded August 27, 2021 11:58

conan-center-bot requested a review from danimtb September 13, 2021 11:57

prince-chrismc mentioned this pull request Sep 19, 2021

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Sep 19, 2021

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Sep 19, 2021

conan-center-bot requested a review from jgsogo September 28, 2021 11:57

jgsogo mentioned this pull request Oct 14, 2021

[gsoap] Move sources + bump deps #7685

Merged

jgsogo reviewed Oct 14, 2021

View reviewed changes

recipes/geotrans/all/conandata.yml Outdated Show resolved Hide resolved

madebr approved these changes Oct 15, 2021

View reviewed changes

jgsogo approved these changes Oct 15, 2021

View reviewed changes

SSE4 approved these changes Oct 15, 2021

View reviewed changes

conan-center-bot merged commit bda1285 into conan-io:master Oct 15, 2021

samuel-emrys deleted the recipe/geotrans branch October 26, 2021 12:41

This was referenced Jan 6, 2023

[request] geotrans/3.9 #15134

Closed

[geotrans] Add version 3.9 #15148

Merged

samuel-emrys mentioned this pull request Jan 15, 2023

[armadillo] Conan 2.0 recipe migration and add v11.4.3 #15109

Merged

3 tasks

Conversation

samuel-emrys commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ruilvo commented Jul 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madebr commented Jul 26, 2021

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jgsogo commented Sep 28, 2021

Uh oh!

samuel-emrys commented Oct 1, 2021

Uh oh!

jgsogo commented Oct 14, 2021

Uh oh!

jgsogo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samuel-emrys commented Oct 14, 2021

Uh oh!

madebr commented Oct 14, 2021

Uh oh!

conan-center-bot commented Oct 14, 2021

Uh oh!

jgsogo left a comment

Choose a reason for hiding this comment

Uh oh!

SSE4 commented Oct 15, 2021

Uh oh!

SSE4 commented Oct 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

samuel-emrys commented Jul 26, 2021 •

edited

Loading

CLAassistant commented Jul 26, 2021 •

edited

Loading