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

jmhCompile should not compile with compiler arg -Werror #866

Merged
merged 10 commits into from
Feb 18, 2020

Conversation

DotSpy
Copy link
Member

@DotSpy DotSpy commented Feb 16, 2020

Jmh fail on compile time because of -Werror flag

@jkwatson
Copy link
Contributor

we should add jmhCompile to the CI steps, so this doesn't happen again. Can you add this to the circle configuration, or to the Makefile, @DotSpy ?

@DotSpy
Copy link
Member Author

DotSpy commented Feb 16, 2020

i would like to get into CI, i'll do it

@DotSpy
Copy link
Member Author

DotSpy commented Feb 17, 2020

@jkwatson done

@@ -3,6 +3,7 @@
.PHONY: test
test:
./gradlew clean assemble check --stacktrace
./gradlew compileJmhJava
Copy link
Member

Choose a reason for hiding this comment

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

I would create a different Makefile target for the benchmarks. This way, we can decouple benchmarks from compilation in CircleCI

Copy link
Member Author

Choose a reason for hiding this comment

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

agree

@DotSpy
Copy link
Member Author

DotSpy commented Feb 17, 2020

Makefile Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #866 into master will increase coverage by 1.8%.
The diff coverage is 84.15%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #866     +/-   ##
===========================================
+ Coverage     78.94%   80.75%   +1.8%     
- Complexity      789      820     +31     
===========================================
  Files           105      108      +3     
  Lines          2845     2899     +54     
  Branches        273      265      -8     
===========================================
+ Hits           2246     2341     +95     
+ Misses          494      452     -42     
- Partials        105      106      +1
Impacted Files Coverage Δ Complexity Δ
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java 91.51% <ø> (ø) 45 <0> (ø) ⬇️
...ntelemetry/exporters/inmemory/InMemoryTracing.java 85.71% <ø> (ø) 5 <0> (ø) ⬇️
...lemetry/sdk/trace/export/SimpleSpansProcessor.java 87.5% <ø> (ø) 7 <0> (ø) ⬇️
...metry/exporters/jaeger/JaegerGrpcSpanExporter.java 67.79% <ø> (ø) 4 <0> (ø) ⬇️
...ava/io/opentelemetry/sdk/trace/SpanBuilderSdk.java 93.87% <ø> (ø) 36 <0> (ø) ⬇️
...o/opentelemetry/sdk/trace/export/SpanExporter.java 100% <ø> (ø) 0 <0> (ø) ⬇️
...elemetry/sdk/trace/export/BatchSpansProcessor.java 89.65% <ø> (+0.86%) 7 <0> (ø) ⬇️
...ava/io/opentelemetry/exporters/jaeger/Adapter.java 96.2% <ø> (ø) 16 <0> (ø) ⬇️
...ntelemetry/sdk/trace/export/MultiSpanExporter.java 100% <ø> (ø) 12 <0> (ø) ⬇️
...java/io/opentelemetry/sdk/trace/data/SpanData.java 100% <ø> (ø) 2 <0> (?)
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc5cfb7...b826c8d. Read the comment docs.

@DotSpy
Copy link
Member Author

DotSpy commented Feb 17, 2020

Any ideas why on CI
circleci.com/gh/open-telemetry/opentelemetry-java/2008?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Task :opentelemetry-api:compileJmhJava SKIPPED ?

In the link you provide, you can find the compileJmhJava task be executed after the build task.

yeap it executed in Build section, but i think that it should be executed in Compile JMH section and not executed in Build

@thisthat
Copy link
Member

Sorry, I wrongly read your comment. It is skipped because there is no /src/jmh/java directory in the API module.

@DotSpy
Copy link
Member Author

DotSpy commented Feb 17, 2020

still if u look at build section it calling > Task :opentelemetry-sdk:compileJmhJava

@DotSpy
Copy link
Member Author

DotSpy commented Feb 17, 2020

Found: gradle assemble calling :opentelemetry-sdk:compileJmhJava

@DotSpy
Copy link
Member Author

DotSpy commented Feb 17, 2020

@jkwatson as u can see here https://circleci.com/gh/open-telemetry/opentelemetry-java/2008#tests/containers/0
gradle assemble calling > Task :opentelemetry-sdk:compileJmhJava
so now we have jmhCompile in our CI, and after i will update #815 from master it will fail.
So question is : Should we move out jmhCompile from gradle assemble and build step in CI in this PR or should we not touch it for now?

@jkwatson
Copy link
Contributor

This seems fine to me.

@DotSpy
Copy link
Member Author

DotSpy commented Feb 18, 2020

in current state there is no sense for Compile JMH step, in it we will get

Task :opentelemetry-sdk:compileJmhJava UP-TO-DATE

because it's called at Build step

@bogdandrutu bogdandrutu merged commit 19713db into open-telemetry:master Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants