Skip to content

Add emitUnitTests.sh#96064

Merged
BruceForstall merged 2 commits intodotnet:mainfrom
a74nh:emitunittests_github
Dec 19, 2023
Merged

Add emitUnitTests.sh#96064
BruceForstall merged 2 commits intodotnet:mainfrom
a74nh:emitunittests_github

Conversation

@a74nh
Copy link
Contributor

@a74nh a74nh commented Dec 15, 2023

The enables the automation of running the emit unit tests for Linux/Mac. Including testing against capstone.

This builds on top of #96005

The intention is for this script to be used during development and eventually added to the CI. I'm not sure what CI conventions I'm missing and I'm not sure if this would require a Windows version.

The enables the automation of running the emit unit tests for Linux/Mac
@ghost ghost added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners community-contribution Indicates that the PR has been added by a community member labels Dec 15, 2023
@a74nh
Copy link
Contributor Author

a74nh commented Dec 15, 2023

@BruceForstall @kunalspathak.

Also, @TIHan @amanasifkhalid as this might be useful for testing your encoding patches.

@a74nh a74nh marked this pull request as ready for review December 15, 2023 14:22
@@ -0,0 +1,115 @@
#!/bin/bash
Copy link
Contributor

@kunalspathak kunalspathak Dec 15, 2023

Choose a reason for hiding this comment

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

Thanks for doing this @a74nh . Perhaps, we should have a python script instead, so it can be used on all platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should be possible to just convert across. this might be a good time to try the Arm64 Windows box we've got.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but IMO having 2 copies for different platform, makes us remember to make changes on both the scripts and soon one might get outdated with respect to other.

this might be a good time to try the Arm64 Windows box we've got

with python, you might not even have to do it ;)

Change-Id: I14896011fbe7aa5be25c425a9a38fa9ec5c56ebd
Copy link
Contributor

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

I think this is fine as-is for its purpose for doing this development. Arguably, it should live in jitutils instead (opinions?).

Having a follow-up so it works on Windows might be useful, but maybe unnecessary depending on the users. And not worth gating this on the conversion. Seems like support for altjit might be useful (e.g., doing development on win-x64 using altjit).

As for putting it in CI, I'm not sure how valuable that would be. It would be problematic to depend on cloning/building a "private" repo (capstone).

@kunalspathak
Copy link
Contributor

As for putting it in CI, I'm not sure how valuable that would be. It would be problematic to depend on cloning/building a "private" repo (capstone).

+1

@a74nh
Copy link
Contributor Author

a74nh commented Dec 18, 2023

Having a follow-up so it works on Windows might be useful, but maybe unnecessary depending on the users

If the others doing the SVE codegen (@TIHan, @amanasifkhalid) would use this on Windows then I'm happy to port. Otherwise, lets keep it as bash then.

As for putting it in CI, I'm not sure how valuable that would be.

My concern was making sure it can easily be re-run, and the code not rot. Doesn't have to go in the CI.

@amanasifkhalid
Copy link
Contributor

If the others doing the SVE codegen would use this on Windows

I develop primarily on Windows, but having a Windows port of this is not a huge priority for me.

Copy link
Contributor

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

Let's go ahead and add merge this as-is.

@BruceForstall BruceForstall merged commit f5f4889 into dotnet:main Dec 19, 2023
@a74nh a74nh deleted the emitunittests_github branch December 19, 2023 09:35
@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants