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

[package] cuda-api-wrappers/0.7: Beta version mistakenly added as non-beta version #22071

Open
eyalroz opened this issue Dec 30, 2023 · 12 comments
Assignees
Labels
bug Something isn't working

Comments

@eyalroz
Copy link
Contributor

eyalroz commented Dec 30, 2023

Description

I'm the author of cuda-api-wrappers. The library was added to conan-center this year (great!) - but without telling me about it (not great).

I have now noticed this happened, but more importantly, I noticed that in #19997 , a beta version, 0.7 beta 1, was added to conan-center as though it was simply version 0.7 - which it is not!

I would like to have that version either removed or marked as beta. I don't know the procedure for doing that. Should I just submit an MR for this? And what is the special marking for beta versions, if any?

I am also worried about how this happened due to some bot action. I will file an infrastructure bug about this mis-handling of beta versions as regular ones (unless you guys tell me not to)

Package and Environment Details

  • Package Name/Version: cuda-api-wrappers/0.7

Conan profile

N/A

Steps to reproduce

N/A

Logs

N/A

@valgur
Copy link
Contributor

valgur commented Dec 30, 2023

Hi @eyalroz. Apologies for the oversight. This was caused by an unfortunate bug in the third-party qchateau/conan-center-bot tool I have been using. I created a bug report for it: qchateau/conan-center-bot#122.

The 0.7 version in the recipe should be renamed to 0.7-b1 or 0.7-beta1 - versions with a hyphenated suffix are interpreted as a pre-release by Conan and don't get matched against in version range checks, for example. See https://docs.conan.io/2/tutorial/versioning/version_ranges.html#semantic-versioning. This is also the format recommended by semver.org: https://semver.org/#spec-item-9.

Unfortunately, I don't think removing or renaming the version in the recipe will eliminate it from the official conan-center repository (but it will be marked as "unmaintained" at least). Maybe @uilianries, @RubenRBS or @danimtb can help out with that?

@eyalroz
Copy link
Contributor Author

eyalroz commented Dec 30, 2023

@valgur : Do you think it's better to have the current PR do just this? Or should I also add the other versions, and update the package meta-data?

Also, FYI: There are other issues, I'll email you about them.

@valgur
Copy link
Contributor

valgur commented Dec 30, 2023

Do you think it's better to have the current PR do just this? Or should I also add the other versions, and update the package meta-data?

Either way is fine, but if the changes you have in mind are non-trivial then splitting them up will allow the current one to be reviewed and merged faster.

@eyalroz
Copy link
Contributor Author

eyalroz commented Dec 30, 2023

Ok, let's start with just replacing 0.7 with 0.7-b1 . Still hoping for any of the three people you @'ed to chime regarding the possibility for really removing the 0.7 package.

@uilianries uilianries self-assigned this Jan 2, 2024
@uilianries
Copy link
Member

Hello @eyalroz ! Sorry the inconvenience with your project! Indeed a PR replacing the version if the best option. I'll talk with Conan team about removing the 0.7 version.

@eyalroz
Copy link
Contributor Author

eyalroz commented Jan 2, 2024

@uilianries : Thanks, please talk to them. More generally, you may also want to bring up two points:

  • For new recipes/library, it is perhaps a good idea to have the PR submitter tell reviewers whether they have contacted the library author and coordinated with them, and if not - why not. This may reduce the amount of redundant effort.
  • It may not be a good idea to easily accept a beta/RC-marked tarball for a non-beta/non-RC version. This situation can be flagged automatically and require some kind of intervention to allow exceptionally.

Anyway, after this PR is merged, I'll soon add some new versions. Although, TBH, I think the recipe may be broken; and the test code is definitely broken.

@uilianries
Copy link
Member

uilianries commented Jan 2, 2024

@eyalroz Thank for your thoughts!

For new recipes/library, it is perhaps a good idea to have the PR submitter tell reviewers whether they have contacted the library author and coordinated with them, and if not - why not. This may reduce the amount of redundant effort.

We have this feature: When someone open a PR that changes a recipe, the CI bot scans a list and ping interested people in the created pull request. I could include you in case changing cuda-api-wrappers. Is it okay for you?

It may not be a good idea to easily accept a beta/RC-marked tarball for a non-beta/non-RC version. This situation can be flagged automatically and require some kind of intervention to allow exceptionally.

We avoid accepting non-stable versions, only for those cases where a beta takes many months til a stable be available. What happened in this case was an automatic operation, when someone open a PR to bump a package version only, the CI bot merges automatically, if it passes. It's hard to track this specific case, because we would need to check the file name, and there is no standard for it. It's possible to list all versions vs packages downloaded, then create a rule to the CI bot.

@uilianries
Copy link
Member

@valgur I'll ask you to not use automated scripts to open a full list of versions to be bumped. As you can see, we have more errors than success rate, and it floods the CI service at the point that we can't prioritize important PRs like Boost. Automation to bump versions was used in the past in CCI and caused more problems than a solution.
I would ask you to please focus your PRs in well tested and important/required versions instead.

@eyalroz
Copy link
Contributor Author

eyalroz commented Jan 2, 2024

We have this feature: When someone open a PR that changes a recipe the CI bot scans a list and ping interested people in the created pull request. I could include you in case changing cuda-api-wrappers. Is it okay for you?

Yes, but note that when a library is added by a third-party - the library author(s) will necessarily not be on this list. So, you don't actually have this feature :-(

because we would need to check the file name, and there is no standard for it

Well, filenames ending with something like [0-9]+[_-]?(rc|a|alpha|b|beta)[_-]?[1-9][0-9]+ followed by the extension, for versions which don't match that suffix, nor do they match release[-_ ]?candidate|alpha|beta - seems like a good initial choice.

@valgur
Copy link
Contributor

valgur commented Jan 2, 2024

@uilianries

I would ask you to please focus your PRs in well tested and important/required versions instead.

I do try to. The last batch of updates was limited to packages that have a newer version available on at least 15 other package repos on Repology. And it was far from automated due to half of them needing actual fixes to get them to work. I don't have any further mass updates in mind, but feel free to ping me anytime if you think I'm overdoing it with the PRs.

I also kind of agree that the automated merging logic could be improved by requiring (1) that the new URL matches one of previous URLs after replacing the old version in it with the new one (2) the version does not contain a pre-release suffix (as suggested by @eyalroz).

Edit: also, I think it would be great if the draft status of PRs could be repurposed to disable the triggering of CI. I like to work on CCI in burst and I would gladly stagger the actual building of the PRs over a much longer period, but currently I would have to set a branch aside and remember to open a PR for it at a later time with the correct details to accomplish this.

@valgur
Copy link
Contributor

valgur commented Jan 2, 2024

Also

TBH, I think the recipe may be broken; and the test code is definitely broken.

After discussing it via email briefly, it is kind of true, in the sense that the recipe does not add any dependencies on CUDA that it actually needs to work. That is unavoidable with CUDA not being available in any shape or form on CCI. I have some ideas for improvement, though. The test_package being nonsensical is simply a consequence of that, as it can only test the minuscule part of the package that does not deal with CUDA.

@eyalroz
Copy link
Contributor Author

eyalroz commented Mar 1, 2024

Please have a look at:

#22947

with more (and more recent) versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants