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

Fix wheels release #68

Merged
merged 16 commits into from
Nov 27, 2024
Merged

Fix wheels release #68

merged 16 commits into from
Nov 27, 2024

Conversation

megaserg
Copy link
Contributor

@megaserg megaserg commented Nov 25, 2024

Use latest cibuildwheel and fix all errors.

@megaserg
Copy link
Contributor Author

@doublethefish this builds all the wheels successfully on my fork :) would appreciate if you could take a look!

@doublethefish
Copy link
Collaborator

Oh awesome!

And it only took 32mins to complete on your fork; did you do anything specific to make it take less than the 5 hours I was seeing?

Copy link
Collaborator

@doublethefish doublethefish left a comment

Choose a reason for hiding this comment

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

It is always better to put more information that "Appease clang" for changes like this.

In future I would like to see the specific "appeasement" that is occurring but in this case I assume it is a warning about template ambiguity.

moved to commit.

@doublethefish doublethefish merged commit afbcaa6 into reymond-group:master Nov 27, 2024
23 of 24 checks passed
@megaserg
Copy link
Contributor Author

Thanks @doublethefish !

Sorry, I wasn't very diligent with the commit messages and history, I was expecting that you'd squash the whole PR 😌

Regarding clang change: it is to fix this error:

        /Users/runner/work/tmap/tmap/src/_tmap/cereal/types/tuple.hpp:119:85: error: a template argument list is expected after a name prefixed by the template keyword [-Wmissing-template-arg-list-after-template-kw]
          119 |     tuple_detail::serialize<std::tuple_size<std::tuple<Types...>>::value>::template apply( ar, tuple );
              |       

which is a relatively recent addition to clang: llvm/llvm-project#80801

Regarding 32 mins: that number is end-to-end but depends on available parallelism (in GitHub free tier?) at the moment. Each job takes about 5 mins (10 mins for macos x86_64) so best case you'd see 10 mins end-to-end, worst case ~2 hours (if everything is sequential).

Thanks again! I believe some of the long-standing issues can be closed now :)

@doublethefish
Copy link
Collaborator

Sorry, I wasn't very diligent with the commit messages and history, I was expecting that you'd squash the whole PR 😌

No worries, thanks for the PR. We are very grateful.

Regarding clang change: it is to fix this error:

Thanks for that it's useful to the specific warning, and edifying to know that I guessed right about the type of warning it was 🤓. As you probably know, -Wmissing-template-arg-list-after-template-kw is a warning that only errors because we have warning-as-errors on for that bit of code. Similar template-specific warnings have been around for a while, and problematic; for instance, at one point fixing this exact issue in one compiler, but leaving spaces between the angle-brackets, on another compiler would cause less-than symbol errors 🤦🏻‍♂️.

@megaserg
Copy link
Contributor Author

Sounds great @doublethefish ! Would you be willing to release the wheels to PyPI?

@doublethefish
Copy link
Collaborator

Yes, of course. But it will have to be sometime over the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants