Skip to content

add initial dotnet support#1598

Merged
asottile merged 1 commit intopre-commit:masterfrom
rkm:feature/dotnet
Sep 27, 2020
Merged

add initial dotnet support#1598
asottile merged 1 commit intopre-commit:masterfrom
rkm:feature/dotnet

Conversation

@rkm
Copy link
Copy Markdown
Contributor

@rkm rkm commented Sep 12, 2020

Hello hello.

This PR adds 2nd class support for dotnet tools by using dotnet pack and dotnet tool install.

I've also tested a "real world" hook:

Details 
$ cd /tmp
$ rm -rf ./foo
$ dotnet new console --name foo
The template "Console Application" was created successfully.

Processing post-creation actions...
Running 'dotnet restore' on foo/foo.csproj...
  Determining projects to restore...
  Restored /tmp/foo/foo.csproj (in 190 ms).

Restore succeeded.

$ cd foo
$ git init
Initialized empty Git repository in /tmp/foo/.git/
$ sed -i 's/Console/    Console/g' Program.cs
$ pre-commit try-repo https://github.com/rkm/format --ref af89478 --files=foo.csproj
[INFO] Initializing environment for https://github.com/rkm/format.
===============================================================================
Using config:
===============================================================================
repos:
-   repo: https://github.com/rkm/format
    rev: af89478
    hooks:
    -   id: dotnet-format
===============================================================================
[INFO] Installing environment for https://github.com/rkm/format.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
dotnet format............................................................Failed
- hook id: dotnet-format
- files were modified by this hook

  Formatting code files in workspace '/tmp/foo/foo.csproj'.
  Warnings were encountered while loading the workspace. Set the verbosity option to the 'diagnostic' level to log warnings.
  Formatted code file '/tmp/foo/Program.cs'.
  Format complete in 3530ms.

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!

@rkm rkm changed the title [WIP] add initial dotnet support add initial dotnet support Sep 12, 2020
asottile
asottile reviewed Sep 12, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@asottile asottile left a 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!

Comment thread pre_commit/languages/dotnet.py Outdated
Comment thread pre_commit/languages/dotnet.py Outdated
Comment thread pre_commit/languages/dotnet.py Outdated
repodir = prefix.path(envdir)

with clean_path_on_failure(repodir):

Copy link
Copy Markdown
Member

@asottile asottile Sep 12, 2020

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

Comment thread pre_commit/languages/dotnet.py Outdated
Comment thread pre_commit/languages/dotnet.py Outdated
if os.path.isfile(d):
os.remove(d)
else:
rmtree(d)
Copy link
Copy Markdown
Member

@asottile asottile Sep 12, 2020

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?

Copy link
Copy Markdown
Contributor Author

@rkm rkm Sep 12, 2020

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.

Copy link
Copy Markdown
Member

@asottile asottile Sep 12, 2020

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?

Copy link
Copy Markdown
Contributor Author

@rkm rkm Sep 13, 2020

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?

Copy link
Copy Markdown
Member

@asottile asottile Sep 15, 2020

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_versions end up in the same clone directory)

Comment thread tox.ini Outdated
@rkm rkm requested a review from asottile September 12, 2020 21:47
@rkm
Copy link
Copy Markdown
Contributor Author

rkm commented Sep 13, 2020

Think I've addressed all your comments now. I've spotted this issue though, so will check if we need to handle that here.

asottile
asottile approved these changes Sep 15, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

looks good! just a few little things and I think this is done (assuming you're happy with it)

Comment thread pre_commit/languages/dotnet.py Outdated
Comment thread pre_commit/languages/dotnet.py
Comment thread pre_commit/languages/dotnet.py Outdated
@rkm
Copy link
Copy Markdown
Contributor Author

rkm commented Sep 26, 2020

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.

asottile
asottile approved these changes Sep 27, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@asottile asottile merged commit 202f0bb into pre-commit:master Sep 27, 2020
@asottile
Copy link
Copy Markdown
Member

asottile commented Sep 27, 2020

would you like to update docs for this? the _new in_ should be 2.8.0

@rkm
Copy link
Copy Markdown
Contributor Author

rkm commented Sep 27, 2020

Sure, will do.

@rkm rkm deleted the feature/dotnet branch December 6, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asottile asottile asottile approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rkm @asottile