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

[invocation-overhead] git ignore generated source files. #1173

Closed
wants to merge 1 commit into from

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Dec 7, 2023

Building the Java.Interop.sln solution always produces some generated files that git wants to try to commit:

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   tests/invocation-overhead/jni.cs

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        tests/invocation-overhead/jni-api.h
        tests/invocation-overhead/jni.c

Ignore these files with .gitignore.

Note that the first time we generate jni.cs we need to manually add it to @(Compile) because the wildcard expansion has already executed at that point. Additional times the file gets regenerated we use KeepDuplicates='false' to prevent a duplicate @(Compile) file warning.

@jpobst jpobst marked this pull request as ready for review December 7, 2023 19:09
@jpobst jpobst requested a review from jonpryor December 7, 2023 19:09
@jonpryor
Copy link
Member

jonpryor commented Dec 11, 2023

We should probably instead add tests/invocation-overhead/jni-api.h and tests/invocation-overhead/jni.c, and update tests/invocation-overhead/jni.cs, as these are useful ways to see what build-tools/jnienv-gen output is, without needing to build the repo itself and look at e.g. src/Java.Interop/obj/Debug/net7.0/JniEnvironment.g.cs or src/java-interop/obj/Debug-net7.0/jni.c.

For example, consider 0f1efeb, which updated jnienv-gen, and also included updates to tests/invocation-overhead/jni.cs. This made code review easier -- at least I'd like to think so -- because the changes introduced to jnienv-gen were visible in the same PR as changes to jni.cs.

@jpobst
Copy link
Contributor Author

jpobst commented Dec 11, 2023

Sounds good to me. This solution also fulfills my desire of appeasing git. 😁

I'll update this PR to do this instead.

@jpobst
Copy link
Contributor Author

jpobst commented Dec 11, 2023

Superseded by #1175.

@jpobst jpobst closed this Dec 11, 2023
@jpobst jpobst deleted the remove-generated branch December 11, 2023 19:55
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants