Skip to content

Conversation

@jonathanpeppers
Copy link
Member

Context: #368

Addressing the comments in #368, this removes more unused build targets, etc.

@jonathanpeppers
Copy link
Member Author

@radekdoulik or @jonpryor Jenkins is running these still (I expect the PR build will probably fail):

make xa-all
make xa-fxcop

Should I go remove these, too?

@radekdoulik
Copy link
Member

I have updated Jenkins configuration to run fxcop instead, so you can remove these.

Copy link
Member

@radekdoulik radekdoulik left a comment

Choose a reason for hiding this comment

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

Besides the xa-all and xa-fxcop rules it looks OK to me.

Makefile Outdated
$(PACKAGES) $(DEPENDENCIES) $(TESTS) $(XA_INTEGRATION_OUTPUTS)

xa-all: $(PACKAGES) $(XA_INTEGRATION_OUTPUTS)
$(PACKAGES) $(DEPENDENCIES) $(TESTS) bin/$(CONFIGURATION)/Java.Interop.dll
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to add bin/$(CONFIGURATION)/Java.Interop.dll to the dependencies list?

Additionally, there's no target to build bin/$(CONFIGURATION)/Java.Interop.dll, so how's this work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, yes, I messed this up.

Context: dotnet#368

Addressing the comments in dotnet#368, this removes more unused build targets, etc.
sed -e 's#@JI_JVM_PATH@#$(JI_JVM_PATH)#g' -e 's#@OS_NAME@#$(DLLMAP_OS_NAME)#g' -e $(JAVA_RUNTIME_ENVIRONMENT_DLLMAP_OVERRIDE_CMD) < $< > $@

fxcop: lib/gendarme-2.10/gendarme.exe bin/GendarmeDebug/Java.Interop.dll
cp src/Java.Interop/obj/Gendarme/Java.Interop.dll.mdb bin/GendarmeDebug/
Copy link
Member

Choose a reason for hiding this comment

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

We still need to copy the mdb file to have lines information in the gendarme output.

Copy link
Member Author

Choose a reason for hiding this comment

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

@radekdoulik there isn't an mdb in here at all now. Maybe a change in the latest mono?

bin/GendarmeDebug/ already has the pdb: Java.Interop.pdb. So is this line not needed anymore?

Copy link
Member

@radekdoulik radekdoulik Sep 12, 2018

Choose a reason for hiding this comment

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

There is an mdb file, check https://jenkins.mono-project.com/job/Java.Interop-pr-builder/ws/src/Java.Interop/obj/Gendarme/

The Gendarme itself is old, and supports only mdb symbol files, thus we use it here until the Gendarme is updated.

I'm not exactly sure how this was working before:
- Gendarme config had no way to import `JdkInfo.props`
- Thus usage of `$(JavaCPath)` was using an empty string?
@radekdoulik radekdoulik merged commit 8d5bcec into dotnet:master Sep 13, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Apr 14, 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.

3 participants