Skip to content

[vtk] FindPEGTL fix#37561

Merged
BillyONeal merged 2 commits intomicrosoft:masterfrom
martinfalk:master
Mar 25, 2024
Merged

[vtk] FindPEGTL fix#37561
BillyONeal merged 2 commits intomicrosoft:masterfrom
martinfalk:master

Conversation

@martinfalk
Copy link
Copy Markdown
Contributor

@martinfalk martinfalk commented Mar 20, 2024

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
Copy Markdown
Contributor Author

martinfalk commented Mar 20, 2024

@microsoft-github-policy-service agree

@Neumann-A
Copy link
Copy Markdown
Contributor

Neumann-A commented Mar 20, 2024

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
Copy Markdown
Contributor

@Neumann-A Neumann-A Mar 20, 2024

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
Copy Markdown
Contributor Author

@martinfalk martinfalk Mar 20, 2024

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
Copy Markdown
Contributor Author

martinfalk commented Mar 20, 2024

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:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. 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
Copy Markdown
Member

@BillyONeal BillyONeal Mar 22, 2024 •
edited
Loading

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
Copy Markdown
Member

@BillyONeal BillyONeal Mar 23, 2024

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
Copy Markdown
Contributor

@Neumann-A Neumann-A Mar 23, 2024

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:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. 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:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Mar 23, 2024
@janicebeinrot
Copy link
Copy Markdown

janicebeinrot commented Mar 25, 2024

@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
@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Mar 25, 2024

Thanks for the fix and sorry I read it backwards before

@martinfalk
Copy link
Copy Markdown
Contributor Author

martinfalk commented Mar 26, 2024

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

+2 more reviewers

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

@JonLiu1993 JonLiu1993 JonLiu1993 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JonLiu1993 JonLiu1993

Labels

category:port-bug The issue is with a library, which is something the port should already support info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

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