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

[vtk] FindPEGTL fix #37561

Merged
merged 2 commits into from
Mar 25, 2024
Merged

[vtk] FindPEGTL fix #37561

merged 2 commits into from
Mar 25, 2024

Conversation

martinfalk
Copy link
Contributor

Fixes #35223

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@martinfalk
Copy link
Contributor Author

@microsoft-github-policy-service agree

@Neumann-A
Copy link
Contributor

Please adjust the pegtl.patch already there instead of adding another one

Neumann-A
Neumann-A reviewed Mar 20, 2024
View reviewed changes
ports/vtk/findpegtl.patch Outdated
Comment on lines 28 to 31
+ get_target_property(TARGET_IMPORTED_GLOBAL taocpp::pegtl IMPORTED_GLOBAL)
+ if(NOT TARGET_IMPORTED_GLOBAL)
+ set_target_properties(taocpp::pegtl PROPERTIES IMPORTED_GLOBAL TRUE)
+ endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to be the only change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the other changes were caused by the automatic conversion of line endings.

@martinfalk
Copy link
Contributor Author

Please adjust the pegtl.patch already there instead of adding another one

Should be fixed now.

@JonLiu1993 JonLiu1993 added the category:port-bug The issue is with a library, which is something the port should already support label Mar 21, 2024
@JonLiu1993 JonLiu1993 mentioned this pull request Mar 21, 2024
Closed
@Neumann-A Neumann-A mentioned this pull request Mar 21, 2024
Merged
1 task
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Mar 22, 2024
BillyONeal
BillyONeal reviewed Mar 22, 2024
View reviewed changes
ports/vtk/pegtl.patch
Comment on lines +74 to +77
+ get_target_property(TARGET_IMPORTED_GLOBAL taocpp::pegtl IMPORTED_GLOBAL)
+ if(NOT TARGET_IMPORTED_GLOBAL)
+ set_target_properties(taocpp::pegtl PROPERTIES IMPORTED_GLOBAL TRUE)
+ endif()
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused: Isn't this identical to just deleting the line set_target_properties(taocpp::pegtl PROPERTIES IMPORTED_GLOBAL TRUE) ?

If taocpp::pegtl is IMPORTED_GLOBAL, then TARGET_IMPORTED_GLOBAL will be true, and we run set_target_properties(taocpp::pegtl PROPERTIES IMPORTED_GLOBAL TRUE) ... but it already was true.

If taocpp::pegtl is not IMPORTED_GLOBAL, then TARGET_IMPORTED_GLOBAL will be false, and we don't do anything.

I think we need to understand why this IMPORTED_GLOBAL was added in the first place?

Copy link
Member

Choose a reason for hiding this comment

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

OK, @ras0219-msft pointed out that I missed a NOT.

Can you explain why this fix works?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the code is run the first time the imported global is not true and as such gets set. Each subsequent run will thus not set the property which generates the error. Due to the property being set the second run will also not create the target and thus it cannot be modified, so the test is need to see if the target was created in this run. It is a bit annoying setting targets to global but some people just mess up their target scoping.

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Mar 22, 2024
@BillyONeal BillyONeal marked this pull request as draft March 22, 2024 23:36
@BillyONeal BillyONeal marked this pull request as ready for review March 22, 2024 23:59
@BillyONeal BillyONeal added the info:reviewed Pull Request changes follow basic guidelines label Mar 23, 2024
@janicebeinrot
Copy link

@martinfalk seems like this PR is ready to merge, hopefully it can go in soon as there is another large PR waiting on this one: #37119 🤞

@BillyONeal BillyONeal merged commit 06e8ed3 into microsoft:master Mar 25, 2024
16 checks passed
@BillyONeal
Copy link
Member

Thanks for the fix and sorry I read it backwards before

@martinfalk
Copy link
Contributor Author

No worries, and happy to contribute :)

JavierMatosD pushed a commit that referenced this pull request Apr 29, 2024
@Neumann-A @BillyONeal
[vtkm|vtk|paraview] update ports (#37119)
3efae11 
closes #36801
closes #36253
closes #36044

~~includes #24327 to fix dlib linkage in the osx pipelines.~~ 

Need to look at 
~~#36044~~
~~#36345~~

merge after 
- [x] #37561

---------

Co-authored-by: Alexander Neumann <you@example.com>
Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BillyONeal BillyONeal BillyONeal left review comments

@Neumann-A Neumann-A Neumann-A left review comments

@JonLiu1993 JonLiu1993 JonLiu1993 approved these changes

Assignees

@JonLiu1993 JonLiu1993

Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[vtk] Duplicate dependencies will result in an error
5 participants
@martinfalk @Neumann-A @janicebeinrot @BillyONeal @JonLiu1993