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 initial dotnet support #1598
Conversation
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.
looks great for a first pass, I'm excited about seeing this land!
pre_commit/languages/dotnet.py
Outdated
repodir = prefix.path(envdir) | ||
|
||
with clean_path_on_failure(repodir): | ||
|
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 should really just write a formatter for this) -- generally blank lines at the top of blocks make it harder to read, I can go through and remove these if you want
pre_commit/languages/dotnet.py
Outdated
if os.path.isfile(d): | ||
os.remove(d) | ||
else: | ||
rmtree(d) |
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 this be fixed by using a temporary directory to build and then just copy the products into the envdir?
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.
Yeah I'm not sure if/how this is done for other languages -- it seems like the hook repo is already cloned before install_environment
is called so there's no chance by that point to clone/build in a temp dir.
I had a minimal version before which removed just the bin
, obj
, and pre-commit-build
dirs since those are the ones we know will exist. I can revert to that if you'd prefer? Or we could skip any cleanup completely. The main reason I added this was that the format
repo I used in the example above is chunky, and all we need after install is the envdir
.
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 think go
does a similar thing and build a temporary build directory and then copies the binaries out of it -- maybe that can be some inspiration?
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.
With go
we seem to specify where the build artefacts will be placed. I'm not sure we can easily do here without doing bota a dotnet build
cmd and then a dotnet pack --no-build
and somehow finding the build output (which can be anywhere really depending on the project layout).
Could we do a git clean
and exclude the final tool directory?
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.
a git clean
would unfortunately preclude doing 1st class in the future so we should avoid that (multiple language_version
s end up in the same clone directory)
Think I've addressed all your comments now. I've spotted this issue though, so will check if we need to handle that here. |
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.
Heyo. I think I'm happy with this now if you are! There will probably be some rough edges found once this starts being used in the wild. Happy for you to ping me in any issues though. |
c2dcd51
to
99d727e
Compare
99d727e
to
003e4c2
Compare
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.
would you like to update docs for this? the |
Sure, will do. |
Hello hello.
This PR adds 2nd class support for dotnet tools by using
dotnet pack
anddotnet tool install
.I've also tested a "real world" hook:
One of the issues I found was that nuget doesn't play nicely when some env vars are not set. In particular the Windows build was failing until I added the tox.ini changes below. If you don't want to carry that addition, then I can probably investigate a workaround using
get_env_patch
.Looking forward to your review!