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

Aggregate bench project #83

Merged
merged 2 commits into from Nov 26, 2020
Merged

Aggregate bench project #83

merged 2 commits into from Nov 26, 2020

Conversation

regadas
Copy link
Collaborator

@regadas regadas commented Nov 24, 2020

With this we propagate compile and fmt tasks/commands.

@codecov-io
Copy link

codecov-io commented Nov 24, 2020

Codecov Report

Merging #83 (8103442) into main (97cbe82) will decrease coverage by 14.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #83       +/-   ##
===========================================
- Coverage   96.14%   82.11%   -14.03%     
===========================================
  Files           5       11        +6     
  Lines         648      794      +146     
  Branches       54       66       +12     
===========================================
+ Hits          623      652       +29     
- Misses         25      142      +117     
Impacted Files Coverage Δ
bench/src/main/scala/cats/parse/bench/atto.scala 0.00% <ø> (ø)
...ch/src/main/scala/cats/parse/bench/fastparse.scala 0.00% <ø> (ø)
...h/src/main/scala/cats/parse/bench/parboiled2.scala 0.00% <0.00%> (ø)
...ench/src/main/scala/cats/parse/bench/parsley.scala 0.00% <0.00%> (ø)
bench/src/main/scala/cats/parse/bench/self.scala 0.00% <0.00%> (ø)
...ch/src/main/scala/cats/parse/bench/JsonBench.scala 0.00% <0.00%> (ø)
... and 2 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 97cbe82...8103442. Read the comment docs.

@regadas regadas force-pushed the aggregate_bench branch 2 times, most recently from 602fdfc to e13966d Compare November 24, 2020 11:30
@regadas
Copy link
Collaborator Author

regadas commented Nov 24, 2020

hm, need to dig a little bit more as this runs locally? 🤔
So CI is running

sbt ++3.0.0-M1 fmtCheck test mimaReportBinaryIssues

but, this is how cross compilation works correctly

sbt fmtCheck "++3.0.0-M1 test" "++3.0.0-M1 mimaReportBinaryIssues"

@regadas regadas force-pushed the aggregate_bench branch 4 times, most recently from 68e4441 to 84cf881 Compare November 25, 2020 22:06
@regadas
Copy link
Collaborator Author

regadas commented Nov 25, 2020

🙌 alright so I guess this is now doing the expected.

Copy link
Collaborator

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

Thanks for dealing with this. I know it can be a frustration to deal with the build.

@@ -55,7 +55,10 @@ jobs:
- name: Check that workflows are up to date
run: sbt ++${{ matrix.scala }} githubWorkflowCheck

- run: sbt ++${{ matrix.scala }} fmtCheck test mimaReportBinaryIssues
- run: |
sbt ++${{ matrix.scala }} fmtCheck \
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need this change? Also, it is interesting there is no quote on the first item.

WorkflowStep.Sbt(List("fmtCheck", "test", "mimaReportBinaryIssues"))
WorkflowStep.Run(
List(
"""sbt ++${{ matrix.scala }} fmtCheck \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is the difference, note the fmtCheck doesn't have quotes around it... But does it need the ++ at all? The format check does not depend on the scala version does it?

Copy link
Collaborator Author

@regadas regadas Nov 26, 2020

Choose a reason for hiding this comment

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

The format check does not depend on the scala version does it?

selecting the scala version on format will allow us to check the format on scala version specific code i.e underscala-2.1x folders. Currently we don't have any so we don't really need it. I thought of separating the "checks" into a separate CI step and run it for one version only but ended up keeping the previous behaviour; can be handy? why not?

The quotes are an interesting case and one of the weirdest behaviours in SBT. When running sbt like this(non interactive) Scala version is not correctly selected on projects when running tasks ... quoting them helps. However, the behavior is not the same on cmds like fmtCheck and leads to error when it expands.

Note: from sbt console you don't need the quotes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the detailed reply.

@johnynek johnynek merged commit 6754c3f into typelevel:main Nov 26, 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.

None yet

3 participants