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

Sort CMake configuration options and update actions/checkout #1151

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

Conversation

keryell
Copy link
Member

@keryell keryell commented Mar 23, 2024

This allows easier comparisons between the various CI configurations.
Use latest GitHub actions/checkout@v4
This fixes some deprecation warnings about old NodeJS.
Also fix some trailing space issues by side effect.

@keryell
Copy link
Member Author

keryell commented Mar 23, 2024

@makslevental @fifield It is unclear to me what is happening here.
I this my fault or just some external planet misalignment?

@makslevental
Copy link
Collaborator

makslevental commented Mar 23, 2024

https://github.com/Xilinx/mlir-aie/actions/runs/8398472772/job/23003372175?pr=1151#step:5:2301

Cloning into '/home/runner/work/mlir-aie/mlir-aie'...
fatal: Remote branch sort-cmake-options not found in upstream origin
Error: Process completed with exit code 128.

I have said this many many many times: there is no point in people that are part of the org (acdc) sending PRs from forks.

Beyond that I would appreciate it if you actually reverted the changes to mlirDistro.yml and mlirAIEDistro.yml (i.e., the two actions that triggered this fail) - they build the distro releases that downstream users depend on (i.e., IREE) and changing the ymls triggers rebuilds.

Finally, the orders of the CMake options weren't arbitrary - they were grouped by (CMake, LLVM, MLIR, AIE). I'm not opinionated about it but to me it was more important to distinguish between generic options (CMake), upstream (MLIR/LLVM) and our options.

This fixes some deprecation warnings about old NodeJS.
Also fix some trailing space issues by side effect.
This allows easier comparison between the various CI configurations.
@keryell
Copy link
Member Author

keryell commented Mar 25, 2024

I have said this many many many times: there is no point in people that are part of the org (acdc) sending PRs from forks.

Sorry, Alzheimer is hitting me at my age. :-(
But otherwise, repository hygiene and preparing for community open-source inclusion come to my mind.

Beyond that I would appreciate it if you actually reverted the changes to mlirDistro.yml and mlirAIEDistro.yml (i.e., the two actions that triggered this fail) - they build the distro releases that downstream users depend on (i.e., IREE) and changing the ymls triggers rebuilds.

Done. Interestingly this was just fixing some NodeJS deprecation.

Finally, the orders of the CMake options weren't arbitrary - they were grouped by (CMake, LLVM, MLIR, AIE). I'm not opinionated about it but to me it was more important to distinguish between generic options (CMake), upstream (MLIR/LLVM) and our options.

I can understand but it is unclear this was fully the case and lexicographic sort is not that bad since the hierarchical prefixes enforce some locality even if it not perfect for some concepts crossing prefixes.
What does the team think about it?

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.

None yet

2 participants