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

FindPystring: Also find a static pystring lib #1449

Merged

Conversation

autoantwort
Copy link
Contributor

This was at least necessary in microsoft/vcpkg#19272

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 5, 2021

CLA Signed

The committers are authorized under a signed CLA.

@autoantwort
Copy link
Contributor Author

Hm it seems that I can also set the option pystring_STATIC_LIBRARY to On. Is there an reason why I have to do it?

@hodoulp
Copy link
Member

hodoulp commented Aug 5, 2021

Hm it seems that I can also set the option pystring_STATIC_LIBRARY to On. Is there an reason why I have to do it?

Discovering a library using the pkgconfig mechanism generates this variable. However, the Findpystring.cmake did not implement the corresponding code so, the variable is never set. Based on the pystring GitHub repo, the pkgconfig was never implemented.

Copy link
Member

@hodoulp hodoulp left a comment

Choose a reason for hiding this comment

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

The fix is the right one.

Following my comment, could you complete the work by cleaning the file i.e. remove lines 44 to 48, and remove the use of ${_pystring_STATIC} ?

@autoantwort
Copy link
Contributor Author

Ok done.

Copy link
Member

@hodoulp hodoulp left a comment

Choose a reason for hiding this comment

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

Thanks @autoantwort for your help.

@autoantwort
Copy link
Contributor Author

Do you know why the pipeline fails? I only see

2021-08-05T16:28:52.6201609Z ##[error]The operation was canceled.

@hodoulp
Copy link
Member

hodoulp commented Aug 10, 2021

@autoantwort here is the link to 'enable' the DCO.

@hodoulp
Copy link
Member

hodoulp commented Aug 17, 2021

Friendly reminder that the pull request could be merged next week so, you could use that period to review the changes.

@autoantwort
Copy link
Contributor Author

Do you mean me? Yeah the PR is ready and can be merged

@hodoulp
Copy link
Member

hodoulp commented Aug 17, 2021

@autoantwort The rule is to wait for 2 approvals or 2 weeks (without pending discussion and one approval) before merging a pull request. So, I raise the point to the community that the delay ends next week. If there is a second approval before the delay I can merge the pull request sooner.

@michdolan michdolan merged commit 3c2c8ba into AcademySoftwareFoundation:master Aug 18, 2021
@hodoulp hodoulp mentioned this pull request Aug 24, 2021
hodoulp added a commit that referenced this pull request Aug 24, 2021
Signed-off-by: Leander Schulten <[email protected]>

Co-authored-by: Patrick Hodoul <[email protected]>
hodoulp added a commit that referenced this pull request Aug 24, 2021
Signed-off-by: Leander Schulten <[email protected]>

Co-authored-by: Patrick Hodoul <[email protected]>
Signed-off-by: Patrick Hodoul <[email protected]>
michdolan added a commit that referenced this pull request Aug 27, 2021
Signed-off-by: Leander Schulten <[email protected]>

Co-authored-by: Patrick Hodoul <[email protected]>
Signed-off-by: Patrick Hodoul <[email protected]>

Co-authored-by: autoantwort <[email protected]>
Co-authored-by: Michael Dolan <[email protected]>
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.

3 participants