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.

Sign up for GitHub

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

Jump to bottom

Install GTSAMConfigVersion.cmake #133

Closed
wants to merge 2 commits into from
Closed

Install GTSAMConfigVersion.cmake #133

wants to merge 2 commits into from

Conversation

Ellon
Copy link
Contributor

@Ellon Ellon commented Oct 4, 2019 •
edited by dellaert
Loading

Hi again,

Unless I missed something I think GTSAM is not installing the GTSAMConfigVersion.cmake, which makes it impossible to specify the version in with for instance find_package(GTSAM 4.0.2 REQUIRED). This happens at least on cmake version 3.14.5.

This PR adds the lines needed to install this file. I also removed the version lines from gtsam_extra.cmake.in since it is now handled by GTSAMConfigVersion.cmake.

Best,

This change is Reviewable

@dellaert dellaert requested a review from varunagrawal October 4, 2019 18:24
@dellaert
Copy link
Member

dellaert commented Oct 4, 2019

@varunagrawal if correct we should merge before moving the tag

varunagrawal
varunagrawal requested changes Oct 4, 2019
View reviewed changes
Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

Solid PR. Needs some updates before I can approve.

cmake/GtsamMakeConfigFile.cmake Outdated Show resolved Hide resolved
gtsam_extra.cmake.in Show resolved Hide resolved
@varunagrawal
Copy link
Collaborator

@Ellon is there a reason your commits are not linking to your github profile? Can you fix that so we can track commits back to owners in the future. :)

@Ellon
Copy link
Contributor Author

Ellon commented Oct 5, 2019 •
edited
Loading

@varunagrawal I probably committed with my new professional email which is not yet linked to my GitHub account (thanks for pointing it out btw!) I'll fix that. :)

Edit: Fixed !

@Ellon Ellon force-pushed the fix/GTSAMConfigVersion.cmake branch 2 times, most recently from 294de2c to 11cce37 Compare October 5, 2019 20:02
@Ellon Ellon requested a review from varunagrawal October 5, 2019 20:04
jlblancoc
jlblancoc reviewed Oct 6, 2019
View reviewed changes
Copy link
Member

@jlblancoc jlblancoc left a comment

Choose a reason for hiding this comment

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

Just some doubts :-)

cmake/GtsamMakeConfigFile.cmake Show resolved Hide resolved
cmake/GtsamMakeConfigFile.cmake Show resolved Hide resolved
cmake/obsolete/FindGTSAM.cmake
include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(GTSAM
REQUIRED_VARS GTSAM_INCLUDE_DIR GTSAM_LIBS
VERSION_VAR GTSAM_VERSION)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to keep this cmake script, which is marked as "obsolete", now that there exist the exported target files + GTSAMConfig.cmake?

Copy link
Contributor Author

@Ellon Ellon Oct 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I also don't see the reason for keeping them. :) I just fixed in case someone still uses them.

Copy link
Member

Choose a reason for hiding this comment

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

@dellaert Should we remove FindGTSAM.cmake? It's an "older" alternative to exported cmake targets, having both is confusing and probably not useful...

@Ellon Ellon force-pushed the fix/GTSAMConfigVersion.cmake branch from 11cce37 to 0592c0c Compare October 6, 2019 10:54
@Ellon Ellon requested a review from jlblancoc October 6, 2019 10:55
@Ellon Ellon force-pushed the fix/GTSAMConfigVersion.cmake branch from 0592c0c to 02d98fd Compare October 6, 2019 10:59
@Ellon Ellon force-pushed the fix/GTSAMConfigVersion.cmake branch from 02d98fd to c6ae958 Compare October 6, 2019 11:02
varunagrawal
varunagrawal approved these changes Oct 6, 2019
View reviewed changes
Copy link
Collaborator

@varunagrawal varunagrawal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is awesome! Pending the resolution of the obsolete cmake files, this PR LGTM.
Thanks a ton @Ellon.

cmake/GtsamMakeConfigFile.cmake Show resolved Hide resolved
cmake/obsolete/FindGTSAM_UNSTABLE.cmake
include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(GTSAM_UNSTABLE
REQUIRED_VARS GTSAM_UNSTABLE_INCLUDE_DIR GTSAM_UNSTABLE_LIBS
VERSION_VAR GTSAM_UNSTABLE_VERSION)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jlblancoc @dellaert if we're deleting the obsolete FindGTSAM.cmake, we should delete this one too.

gtsam_extra.cmake.in Show resolved Hide resolved
@Ellon
Copy link
Contributor Author

Ellon commented Oct 7, 2019

@varunagrawal You're welcome. :)

If you're going to delete the obsolete files, feel free to merge only the first commit. ;)

Best,

@jlblancoc
Copy link
Member

Merged via git cherry picking: 3fad1fa
and removed the two obsolete files: bbf007e

There are a few other files under "cmake/obsolete", but I'm not sure of someone is still using them, so won't touch them.

Thanks @Ellon !

@jlblancoc jlblancoc closed this Oct 7, 2019
@Ellon
Copy link
Contributor Author

Ellon commented Oct 7, 2019

Np @jlblancoc !

Not sure if it was on purpose, but you cherry-picked the commit on top of develop, while I proposed changes to master. A kind warning just in case. :)

Best,

@jlblancoc
Copy link
Member

Hmm.... AFAIK, gtsam has been using "develop" as what most other projects use "master". Looking at the commit history, there are almost no changes to "master" in the past months, virtually all went straight to "develop".

@dellaert @varunagrawal @chrisbeall : What's the role of "master" vs. "develop" in this repository? Just to be sure of not messing around :-)

@Ellon
Copy link
Contributor Author

Ellon commented Oct 7, 2019

IMO develop is for new features and master is more for stable releases, as for example the release tags 4.0.0, 4.0.1 and 4.0.2 follows this branch.

That's why I proposed the merge on master, because I see the PR as a fix (that could be applied on 4.0.2 if the tag is moved after the merge, see #130 ), and not as a feature. :)

@varunagrawal
Copy link
Collaborator

@Ellon is right about the difference between develop and master. We use GitFlow where all development happens on develop and master only records releases. From this logic, the commits of this PR should go into develop directly and then pulled into master for 4.0.3 or whatever the next release is.

That being said, thanks @Ellon and @jlblancoc for landing this PR really quickly. This is incredibly helpful work in terms of versioning for GTSAM.

@varunagrawal varunagrawal mentioned this pull request Oct 7, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varunagrawal varunagrawal varunagrawal approved these changes

@jlblancoc jlblancoc jlblancoc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ellon @dellaert @varunagrawal @jlblancoc