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

Add gz ports: [gz-cmake3] [gz-utils2] [gz-tools2] [gz-math7] #28656

Merged
merged 2 commits into from May 1, 2023

Conversation

talregev
Copy link
Contributor

Describe the pull request
Add gz ports

  • What does your PR fix?

    Fixes #...

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    <all / linux, windows, ...>, <Yes/No>

  • Does your PR follow the maintainer guide?

    Your answer

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes
If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

github-actions[bot]
github-actions bot previously approved these changes Dec 31, 2022
@Cheney-W Cheney-W added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jan 3, 2023
@Cheney-W
Copy link
Contributor

Cheney-W commented Jan 3, 2023

@talregev
Copy link
Contributor Author

talregev commented Jan 3, 2023 via email

@talregev
Copy link
Contributor Author

talregev commented Jan 3, 2023

Also for each version of gazebo ignition lib part, we created a new port.
I think the reason that it is like that it for support old stuff here.
You can see for these ports already it is separated by the version .

@Cheney-W Cheney-W added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jan 4, 2023
@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 4, 2023
@vicroms
Copy link
Member

vicroms commented Jan 10, 2023

Are ignition ports side-by-side installable with these gz ports? If not, then we need to remove the old ones or we could introduce a lot of conflicts.

I think it would be better to move the ignition ports to their own registry if you still want to provide support for them.

@talregev
Copy link
Contributor Author

Are ignition ports side-by-side installable with these gz ports? If not, then we need to remove the old ones or we could introduce a lot of conflicts.

I think it would be better to move the ignition ports to their own registry if you still want to provide support for them.

The ignition ports can installable side by side with gz-ports.
This is not my will to preserve the ignition port aka old ports. I am not sure other ports will effect by them.
Currently I know the old ports needed for old gazebo compilation. I am trying to compile the new gazebo, step by step, port after port.

I didn't understand your suggestions for "own registry".

@Cheney-W Cheney-W removed the info:reviewed Pull Request changes follow basic guidelines label Jan 11, 2023
@vicroms
Copy link
Member

vicroms commented Jan 12, 2023

I meant creating a custom registry that contains the ignition ports, that way we can delete them from the vcpkg main catalogue, but you'll still be able to maintain the old ports if you want on their own registry.

See our registries documentation to learn how to create a custom registry and promote it in the public registry collection.

@vicroms vicroms removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 12, 2023
@talregev
Copy link
Contributor Author

contains the ignition ports, that way we can delete them from the vcpkg main catalogue, but you'll still be able to maintain the old ports if you want on their own registry.

Thank you for your explanation. I don't have time to do this. Can you help me?

@talregev
Copy link
Contributor Author

talregev commented Jan 13, 2023

There already old ignition ports with their version number.
What do you want to do with them?

image

@talregev
Copy link
Contributor Author

talregev commented Jan 17, 2023

@vicroms I don't wish to register the previous ports. I don't have use of them.
What are you suggesting to do with ignition ports? Are you wish to delete them?
Some of them depends in other ports. for example gazebo port.

@talregev
Copy link
Contributor Author

talregev commented Feb 3, 2023

@vicroms @BillyONeal
What you decided to do with the ports with older version that have common name for the same lib?

github-actions[bot]
github-actions bot previously approved these changes Feb 3, 2023
@talregev talregev mentioned this pull request Feb 3, 2023
@talregev talregev changed the title Add gz ports: [gz-cmake3] [gz-utils2] [gz-tools2] [gz-math7] Add gz ports: [gz-cmake3] [gz-utils2] [gz-tools2] [gz-math7] [sdformat13] Feb 3, 2023
@talregev
Copy link
Contributor Author

talregev commented Feb 3, 2023

I added sdformat13 to this PR.

github-actions[bot]
github-actions bot previously approved these changes Feb 3, 2023
@talregev
Copy link
Contributor Author

talregev commented Feb 3, 2023

I remove sdformat13 because I didn't succeeded to compile it on linux and mac.
I will try in other when this port will be merge.

@reitowo
Copy link
Contributor

reitowo commented Mar 13, 2023

Blocking #30135

@talregev
Copy link
Contributor Author

Blocking #30135

Can you elaborate?

@Cheney-W Cheney-W added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Mar 13, 2023
@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Mar 21, 2023
@BillyONeal
Copy link
Member

This all seems reasonable to me and I'm not sure why it's marked requires team review. I'll have an answer for ya worst case on Thursday after our maintainer meeting.

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Apr 12, 2023
@complexlogic
Copy link
Contributor

@BillyONeal Any update? This PR is blocking the FFmpeg 6 update, and I would like to use that in my projects.

@BillyONeal
Copy link
Member

@BillyONeal Any update? This PR is blocking the FFmpeg 6 update, and I would like to use that in my projects.

I'm still in a holding pattern because I haven't been able to hear from Victor, he's got IRL stuff going on. I don't want to just merge without talking to him because he tagged with the 'requires review' tag.

BillyONeal
BillyONeal previously approved these changes Apr 27, 2023
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

OK we finally talked about this. (@vicroms, @JavierMatosD, @dan-shaw, and I)

The vcpkg-team-review concern is: upstream no longer cares about maintaining ignition-common3, and continuing to index that in the curated registry potentially blocks other unrelated updates like ffmpeg6 #30135

Note that only merging this PR will not unblock the ffmpeg6 update, because the other ignition ports remain listed.

Potential outcomes:

  • No, the vcpkg port names use ignition- and we should pick only one despite side-by-side support upstream.
  • Delist the older ignition ports.
  • Baseline older ignition ports.
  • Maintain patches forever.

Result:

Speaking personally I think we should delist the ignition- names in the ffmpeg6 update PR.

@BillyONeal BillyONeal added info:reviewed Pull Request changes follow basic guidelines and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Apr 27, 2023
@BillyONeal BillyONeal dismissed stale reviews from Cheney-W, github-actions, and themself via e9fc242 April 27, 2023 22:10
@traversaro
Copy link
Contributor

Contact @traversaro for opinions on the above potential outcomes because they are the original contributor of the ignition ports.

Replied in #30135 (comment) .

@BillyONeal BillyONeal merged commit 197818b into microsoft:master May 1, 2023
15 checks passed
@BillyONeal
Copy link
Member

Thanks!

@talregev talregev deleted the TalR/gz_ports branch May 5, 2023 20:42
@talregev talregev restored the TalR/gz_ports branch May 5, 2023 20:44
@talregev talregev deleted the TalR/gz_ports branch May 5, 2023 20:44
@talregev talregev restored the TalR/gz_ports branch May 5, 2023 20:44
@talregev talregev mentioned this pull request Aug 17, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants