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

Fixes FindQwt.cmake on archlinux #2545

Closed
wants to merge 1 commit into from

Conversation

sab24
Copy link

@sab24 sab24 commented Jun 10, 2019

Hi,

I tried to update the FindQwt.cmake file to find the version number in the file /usr/lib/libqwt.so on archlinux that does not have the version number in the filename like for example qwt6-[version]

I hope this improves the cmake file. I have only tested it on archlinux.

@michaelld
Copy link
Contributor

@sab24 Thanks for your work here! Please post when this PR is ready to go in your opinion && we'll review it.

@sab24
Copy link
Author

sab24 commented Jun 11, 2019

Hi @michaelld, thanks for the reply. I tested it on archlinux, there it builds fine and all tests pass. Also on buildbot it is working, so I think it is ready for review.

@michaelld
Copy link
Contributor

@sab24 great! Will do.

@mbr0wn mbr0wn added the CMake label Jun 15, 2019
Copy link
Member

@mbr0wn mbr0wn left a comment

Choose a reason for hiding this comment

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

Code looks fine. Can you squash the commits?

@sab24
Copy link
Author

sab24 commented Jun 16, 2019

Code looks fine. Can you squash the commits?

Thanks for the review. I have no idea how to squash them, I tried, it only added more commits :(

@mbr0wn
Copy link
Member

mbr0wn commented Jun 16, 2019

@sab24 You can git rebase -i your branch onto master, and then squash your commits that way.

@sab24 Do we have a CLA for you? @marcusmueller Does this require one? Seems long-ish.

…n its name and checks version of headers and library. Fixes issue on Arch Linux with QWT 6
@sab24
Copy link
Author

sab24 commented Jun 17, 2019

@sab24 You can git rebase -i your branch onto master, and then squash your commits that way.

@sab24 Do we have a CLA for you? @marcusmueller Does this require one? Seems long-ish.

@mbr0wn, fixed, had to push --force to push rebased commits remotely. According to your policy more than 10 lines requires a copyright assignment. Can you provide me instructions on how to do so?

@mbr0wn
Copy link
Member

mbr0wn commented Jun 18, 2019

@sab24 Thanks for squashing! push --force was the correct choice. For the CLA, please contact bhilburn at gnuradio dot org. Thank you so much!

Copy link
Contributor

@michaelld michaelld left a comment

Choose a reason for hiding this comment

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

Small change. See my comment.

@@ -29,7 +29,7 @@ find_path(QWT_INCLUDE_DIRS
)

find_library (QWT_LIBRARIES
NAMES ${PC_QWT_LIBRARIES} qwt6-${QWT_QT_VERSION} qwt-${QWT_QT_VERSION}
NAMES qwt ${PC_QWT_LIBRARIES} qwt6-${QWT_QT_VERSION} qwt-${QWT_QT_VERSION}
Copy link
Contributor

Choose a reason for hiding this comment

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

In my testing on OSX with both Qwt52 and Qt5Qwt6 installed, putting stock "qwt" here first will result in -any- libqwt that's found being used (in my case, Qwt52), which is not desirable. What we want is for this entry to be last (or at least not first), so that whatever (if anything) PC found is found first, then the other names, then this.

Copy link
Member

Choose a reason for hiding this comment

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

Good point! PC must always come first, or it's not being used.

Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly needs to be done here?

@mbr0wn
Copy link
Member

mbr0wn commented Jan 6, 2020

We have both merge conflicts, and the "Needs CLA" label has been lingering for too long. I'm closing this. However, that's more for the PR queue to be not too crazy. Please reopen or resubmit once the CLA is in, and the PR is working again.

@mbr0wn mbr0wn closed this Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants