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 package should add the modules to CMAKE_MODULE_PATH #587

Closed
wants to merge 2 commits into from

Conversation

Lectem
Copy link

@Lectem Lectem commented Feb 5, 2020

Issue: awslabs/aws-c-event-stream#15

Right now aws-c-event-stream needs a lot of boilerplate code to determine the modules installation path while the aws-c-common package should provide it.
This would fix awslabs/aws-c-event-stream#15 simply by moving find_package(aws-c-common REQUIRED) before the various module includes
Using configure_package_config_file makes it easier to have a relocatable package.

A pull request will be opened in aws-c-event-stream too.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Right now aws-c-event-stream needs a lot of boilerplate code to determine the modules installation path while the aws-c-common package should provide it.
This would fix awslabs/aws-c-event-stream#15 simply by moving `find_package(aws-c-common REQUIRED)` before the various module `include`s
Using `configure_package_config_file` makes it easier to have a relocatable package.
Lectem added a commit to Lectem/aws-c-event-stream that referenced this pull request Feb 5, 2020
`aws-c-event-stream` should not need to worry nor now how to find the `aws-c-common` CMake modules.
This can be fixed by simply moving up `find_package(aws-c-common REQUIRED)` and making the `aws-c-common` package set the paths correctly.

This has been confirmed to work on amazon linux 2.

This commit requires awslabs/aws-c-common#587 to be merged first.
@Lectem
Copy link
Author

Lectem commented Feb 18, 2020

Any news on this ?
I'm waiting for this PR and awslabs/aws-c-event-stream#33 to be merged so that we can set our CI back to upstream and get the latest updates.

@JonathanHenson
Copy link
Contributor

I think this is a great change, but we'd need to apply it evenly across all the repos (there's 8 more), so it will take time. It's also a lot of work when we get the cmake stuff wrong to get them upgraded. We're evaluating this and we'll apply it next time we touch our cmake.

As for the linked issue, it looks like your code is out of date. The actual bug was fixed a while ago.

Are you having problems with the latest commit?

@Lectem
Copy link
Author

Lectem commented Feb 26, 2020

As for the linked issue, it looks like your code is out of date. The actual bug was fixed a while ago.

Are you having problems with the latest commit?

Sorry I missed the question last time.
Yes I was having issues when building on amazon linux 2, using the latest commits at the time. That would be 60d5f8d.

Should I try with latest master ?

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.

AWS C/C++ packages use non-standard cmake module paths breaking most distributions
3 participants