-
Notifications
You must be signed in to change notification settings - Fork 281
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 support for Windows and revamp FIRRTL release artifacts #5470
Conversation
tar: tar | ||
cc: clang | ||
cxx: clang++ | ||
cmake-args: '-DCMAKE_C_COMPILER=clang-12 -DCMAKE_CXX_COMPILER=clang++-12' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the compiler override here because I couldn't figure out what the name of the compiler was for Windows 😂. Open to moving it back but it is also nice having MacOS and Windows just use the default rather than having to also specify it for them.
#Checks temporarily disabled because current LLVM commit (9305b63d6) fails checks | ||
#ninja check-llvm check-mlir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted because they were commented out anyway and they still don't work. @dtzWill may have thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we run check-mlir
? Dropping is fine (as you say they were commented out anyway), but these are configurations not built/tested elsewhere, and as shown by the whole clang-12 thing things can break unique to these environments.
Just like Chisel tests don't substitute CIRCT's own testing (nor should they), MLIR can be broken in ways our tests wouldn't expose but users may encounter.
It's unfortunate we can't easily more narrowly run the tests for the dependencies we care about, but check-mlir
seems reasonable since we do use a fair bit of MLIR but a much smaller fraction of LLVM (re:test failures not being on components we care about).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added ninja check-mlir
. I've spun up a couple of runs.
I think if we're going to test mlir here we really need to test it elsewhere because finding out that check-mlir
is broken after tagging a release is not ideal (but I grant we should check it here as well).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this, for adding Windows, and publishing the hashes!
Few comments but generally LGTM!
#Checks temporarily disabled because current LLVM commit (9305b63d6) fails checks | ||
#ninja check-llvm check-mlir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we run check-mlir
? Dropping is fine (as you say they were commented out anyway), but these are configurations not built/tested elsewhere, and as shown by the whole clang-12 thing things can break unique to these environments.
Just like Chisel tests don't substitute CIRCT's own testing (nor should they), MLIR can be broken in ways our tests wouldn't expose but users may encounter.
It's unfortunate we can't easily more narrowly run the tests for the dependencies we care about, but check-mlir
seems reasonable since we do use a fair bit of MLIR but a much smaller fraction of LLVM (re:test failures not being on components we care about).
07721af
to
4901752
Compare
* Add Windows (firtool.exe) * Remove Ubuntu 18.04 (runner is deprecated) * Name archives based on OS and architecture * linux-x64, macos-x64, linux-x64 * Windows uses .zip, Linux and MacOS use .tar.gz * Add sha256 hashes for archives
4901752
to
4ad55c7
Compare
Waiting to see how https://github.com/jackkoenig/circt/releases/tag/firtool-1.45.0-ish goes before merging. |
* Add Windows (firtool.exe) * Remove Ubuntu 18.04 (runner is deprecated) * Name archives based on OS and architecture * linux-x64, macos-x64, linux-x64 * Windows uses .zip, Linux and MacOS use .tar.gz * Add sha256 hashes for archives
* [CI] Update github actions for checkout, cache: v2 -> v3 (#5268) Node12 -> Node16, primarily. See: https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/ . Appears safe, both have some improvements we may enjoy, such as zstd for Windows cache. * Revamp FIRRTL release artifacts (#5470) * Add Windows (firtool.exe) * Remove Ubuntu 18.04 (runner is deprecated) * Name archives based on OS and architecture * linux-x64, macos-x64, linux-x64 * Windows uses .zip, Linux and MacOS use .tar.gz * Add sha256 hashes for archives --------- Co-authored-by: Will Dietz <will.dietz@sifive.com>
I tried to keep the build flows as similar as possible, which included making the cmake commands work across Linux/Mac (using bash) and Windows (using powershell). Windows does have bash and it is called out as the shell on a few build steps, but
./utils/find-vs.ps1
is written in powershell and is needed to set up the Visual Studio environment needed to build firtool so I had to keep the cmake steps as powershell.You can see what the new artifacts look like here https://github.com/jackkoenig/circt/releases/tag/firtool-1.44.0-jack (at time of opening this PR, things are building, so until it's done look here instead).
I based the tweaking naming scheme on what is used by GraalVM and Adoptium OpenJDK. I'm open to other schemes but I think this one seems pretty good.