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 a noqt package and variants noqt5, noqt6 #25688

Merged
merged 8 commits into from
Oct 20, 2024
Merged

Conversation

hmaarrfk
Copy link
Contributor

See conda-forge/opencv-feedstock#293 and more recent interest: #25669

@conda-forge/qt-main requests for comments and thoughts is greatly appreciated

@ocefpaf one of my concerns was about "maintainability" of this package. It seems that the only way to update such a package is through repodata patches. As such, I decided to create out of an abundance of caution a noqt4 package. Let me know what you think.

cc: @traversaro @jschueller

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/noqt) and found some lint.

Here's what I've got...

For recipes/noqt:

  • The following maintainers have not yet confirmed that they are willing to be listed here: conda-forge/qt-main. Please ask them to comment on this PR if they are.

@hmaarrfk hmaarrfk changed the title Add a noqt5 package and variants Add a noqt package and variants noqt4, noqt5, noqt6 Mar 10, 2024
# qt-main is essentially qt5
- qt-main <0.a0
# before the creation of qt-main, a megapackage called qt existed
- qt <0.a0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this be loosened to qt <5.a0 to allow for qt4 to be installed? i feel like that will cause a whole set of other problems.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly, hence why I think the distinction between noqt5 and noqt4 isn't worth it.

@ocefpaf
Copy link
Member

ocefpaf commented Apr 16, 2024

@ocefpaf one of my concerns was about "maintainability" of this package. It seems that the only way to update such a package is through repodata patches. As such, I decided to create out of an abundance of caution a noqt4 package. Let me know what you think.

For some odd reason I missed this ping. I'm OK with it. I don't see any obvious downsides.

Copy link

stale bot commented Sep 25, 2024

Hi friend!

We really, really, really appreciate that you have taken the time to make a PR on conda-forge/staged-recipes! conda-forge only exists because people like you donate their time to build and maintain conda recipes for use by the community.

In an effort to maintain this repository and increase the signal-to-noise for open PRs, the maintainers of staged-recipes close excessively old PRs after six months. This PR will remain open for another month, and then will be closed.

If you'd like to keep it open, please comment/push and we will be happy to oblige! Note that very old PRs will likely need to be rebased on main so that they can be rebuilt with the most recent CI scripts. If you have any trouble, or we missed reviewing this PR in the first place (sorry!), feel free to ping the team using a special command in a comment on the PR to get the attention of the staged-recipes team.

Cheers and thank you for contributing to this community effort!

@stale stale bot added the stale will be closed in 30 days label Sep 25, 2024
Copy link
Contributor

Hi! This is the staged-recipes linter and I found some lint.

File-specific lints and/or hints:

  • recipes/noqt/meta.yaml:
    • lints:
      • The following maintainers have not yet confirmed that they are willing to be listed here: conda-forge/qt-main. Please ask them to comment on this PR if they are.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/noqt/meta.yaml) and found it was in an excellent condition.

@hmaarrfk
Copy link
Contributor Author

I've reopened this because it seems like it may help at compilation to resolve environments.
conda-forge/freecad-feedstock#122

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Thanks for coming up with this! I think the idea is great, and it would be great to have for certain build scenarios! Some comments below.

Comment on lines 24 to 20
build:
noarch: generic
requirements:
run_constrained:
# qt4 only ever had a "mega qt" package called qt
- qt <0.a0|>=5.a0
test:
commands:
- echo "no tests for this package"
Copy link
Member

Choose a reason for hiding this comment

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

I would skip noqt4 completely. qt4 is so old (the last build was >5 years ago) that it's essentially uninstallable anyway.

With noqt5 requiring both qt <0.0a0 and qt-main <0.0a0, that already encompasses all remnants of qt4 that might exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. i removed noqt4.

Comment on lines 38 to 41
# qt-main is essentially qt5
- qt-main <0.a0
# before the creation of qt-main, a megapackage called qt existed
- qt <0.a0
Copy link
Member

@h-vetinari h-vetinari Oct 13, 2024

Choose a reason for hiding this comment

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

We need a minor version before the "a0". Also, I would only do qt-main <5.0a0, so we don't close the door that qt6-main eventually becomes qt-main. That the output got renamed was to enable co-installation; once qt5 fades away, we won't need that anymore. Not saying that's a good enough reason to then change it, but it might happen.

Suggested change
# qt-main is essentially qt5
- qt-main <0.a0
# before the creation of qt-main, a megapackage called qt existed
- qt <0.a0
# qt-main only exists as of 5.x
- qt-main <5.0a0
# before the creation of qt-main, a megapackage called qt existed
- qt <0.0a0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks o hope i addressed the 2 points.

Comment on lines 50 to 52
run_constrained:
# For qt6, we always had qt6-main
- qt6-main <0.a0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
run_constrained:
# For qt6, we always had qt6-main
- qt6-main <0.a0
run_constrained:
# For qt6, we always had qt6-main
- qt6-main <0.0a0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thnaks.

# qt-main is essentially qt5
- qt-main <0.a0
# before the creation of qt-main, a megapackage called qt existed
- qt <0.a0
Copy link
Member

Choose a reason for hiding this comment

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

Exactly, hence why I think the distinction between noqt5 and noqt4 isn't worth it.

@h-vetinari
Copy link
Member

one of my concerns was about "maintainability" of this package. It seems that the only way to update such a package is through repodata patches.

What do you mean here? Like if the structure of the qt packages changes, we need to modify the repodata? I think that's an acceptable price if we end up doing big changes (which would have to be well-justified in and of themselves anyway).

@hmaarrfk
Copy link
Contributor Author

I mostly mean helping to reduce the scope of this.

For now, that means removing the noqt4 package.

Comment on lines 26 to 27
# qt-main only exists as of 5.x
- qt-main <0.0a0
- qt-main <5.0a0|>=6.0a0
Copy link
Member

Choose a reason for hiding this comment

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

I remember some discussion in mamba recently about these kind of "or" specifiers. Can we get confirmation that they're fully supported in mamba (@jjerphan @JohanMabille) & pixi (@wolfv @baszalmstra)?

Copy link
Member

Choose a reason for hiding this comment

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

The | operator is supported by mamba.

For instance see this test out of many more in the same file.

IIRC, the discussion were around another MatchSpec, namely openblas 0.2.18|0.2.18.*. for which the trailing *. segment is not properly supported.

Copy link
Member

Choose a reason for hiding this comment

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

Fully supported by rattler/py-rattler/pixi/rattler-build as well.

@h-vetinari h-vetinari changed the title Add a noqt package and variants noqt4, noqt5, noqt6 Add a noqt package and variants noqt5, noqt6 Oct 14, 2024
Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@h-vetinari
Copy link
Member

I think we're good to merge here?

@hmaarrfk
Copy link
Contributor Author

yes, feel free to press merge, but i don't think it resolves your main concern. (more info on the original feedstock that brought us here)

hmaarrfk and others added 5 commits October 20, 2024 10:05
See conda-forge/opencv-feedstock#293
and more recent interest: conda-forge#25669

@conda-forge/qt-main requests for comments and thoughts is greatly
appreciated

@ocefpaf one of my concerns was about "maintainability" of this package.
It seems that the only way to update such a package is through repodata
patches. As such, I decided to create out of an abundance of caution a
noqt4 package. Let me know what you think.
Co-authored-by: h-vetinari <h.vetinari@gmx.com>
Co-authored-by: h-vetinari <h.vetinari@gmx.com>
@hmaarrfk
Copy link
Contributor Author

I think this can at least help CIs avoid a qt download. users can add "noqt" to help the solving from finding qt.

@hmaarrfk hmaarrfk merged commit a1fa513 into conda-forge:main Oct 20, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants