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

Updating repository to follow calng-format-wheel #88

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

CalebFuster
Copy link

Updated repository to follow ssciwr/clang-format-wheel to ease maintenance.

Update include:

Moving from legacy setup.py to pyproject.toml
    Removing Manifest
CMakeList update to follow updates on clang-format-wheel
    Renaming clang-tidy_version.cmake to clang-tidy_version.txt
        Changing version to vX.X.X.X

@CalebFuster
Copy link
Author

The PR is unfinished as GitHub action requires some changes, but there are some minor doubts about the code as it is.

Header files added to the wheel.

Why are they necessary for the wheel? Probably I'm overseeing something there.

install(
  DIRECTORY
    ${CMAKE_BINARY_DIR}/llvm/lib/clang/${CLANG_TIDY_VERSION_MAJOR}/include
  DESTINATION clang_tidy/data/lib/clang/${CLANG_TIDY_VERSION_MAJOR}
)

Visual Studio

I don't use VS, but I assume this is useful here for whom uses it.

set(config-subfolder "")
if(CMAKE_GENERATOR MATCHES "Visual Studio")
  set(config-subfolder "Release")
endif()
set(clang-tidy-executable ${CMAKE_BINARY_DIR}/llvm/${config-subfolder}/bin/clang-tidy${CMAKE_EXECUTABLE_SUFFIX})

@CalebFuster
Copy link
Author

About GitHub action for release. I have some experience, but I prefer to ask

Minor improvements

OS runners

Clang-format-wheel uses Ubuntu:latest but not latest versions for macOS and windows. I would suggest (at least for this repo) moving all them to use latest, so deprecation OS runners don't affect. MacOS-12 is part of the workflows of this repo, and it's deprecated Issue. However, as you compile for Intel and Arm64 macOS, it will require maintenance sooner rather than later.

Automatic workflow updates with dependent bot.

There are a couple of PR from dependent bot #84 #85 #86. I can modify them manually in the PR, or you merge them and I pull them.

Excluded OS compilations

Some of the combinations are commented, but we can use the strategy of clang-format-wheel repo. Additionally, modify the default answer to the simulation to false, so we skip them by default.

About this PR

This part will take some time to be finished as there are a lot of small changes to test, running act with this workflow release leads to BSOD (expected as there are like 6 different targets to compile). I can add any of the previous modifications if you find them useful. Once the PR is merged, I would do a final test with a release of 19.1.6 for example.

@renefritze
Copy link
Collaborator

Looking good so far @CalebFuster, thanks!
I wasn't even aware the clang-format wheel moved on to scikit-build-core :)

Why are they necessary for the wheel?

Don't remember. If you're keen to know, have you looked at the git blame of the file? Maybe the adding commit has some notes.

OS runners

Agree on moving to what is currently latest. I just generally try to avoid using the X-latest tag

Automatic workflow updates with dependent bot.

I was hesitant to merge #86 since that potential for needing changes in the workflow.

Same with #76, this has a very low compat score.

So it would be great if you can trial those changes in your fork.

Excluded OS compilations
Some of the combinations are commented, but we can use the strategy of clang-format-wheel repo.

Not quite sure what you're saying here. Do you want to try to re-enable some currently commented combinations?

Name and others added 2 commits January 18, 2025 13:23
tmp: Std folder to add files in repos for testing and other stuff.
wheelhouse: Std output of builds with cibuildwheel. We can avoid to add them by mistake when debugging locally or with Docker
IDE config folders: Everyone has their own way of doing things. Good to ignore configs
@CalebFuster
Copy link
Author

Don't remember. If you're keen to know, have you looked at the git blame of the file? Maybe the adding commit has some notes.

I found the reason the headers are need for some of the checks.

Do you want to try to re-enable some currently commented combinations?

I wanted to remove the comment on the qemu combinadtion and run them conditionally as (clang-format workflow). However, the workflow was not conditional and it triggered all the combinations.

About dependant bot, qemu is not use anyway so we can remove it in the meantime. About pypi upload, I will try tomorrow to test it against test pypi, but quite confident on it.

Comment on lines -95 to -97
- name: Set up QEMU
uses: docker/[email protected]
if: runner.os == 'Linux' && ((matrix.use_qemu) && fromJSON(env.USE_QEMU))
Copy link
Author

Choose a reason for hiding this comment

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

I removed the qemu with the commented combination

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