Skip to content

Compile root in CI #3425

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 3, 2017
Merged

Compile root in CI #3425

merged 2 commits into from
Nov 3, 2017

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki nicolasstucki force-pushed the ensure-all-is-compiled-in-ci branch from 56b5ca3 to ea63d97 Compare November 1, 2017 16:25
@nicolasstucki nicolasstucki changed the title Compiler root in CI Compile root in CI Nov 1, 2017
allanrenucci
allanrenucci previously approved these changes Nov 1, 2017
View reviewed changes
project/Build.scala Outdated
@@ -1134,6 +1140,7 @@ object Build {
bootstrappedAggregate(`scala-library`, `scala-compiler`, `scala-reflect`, scalap, `dotty-language-server`).
dependsOn(dottyCompiler).
dependsOn(dottyLibrary).
dependsOn(dottyBench). // just to make sure it compiles
Copy link
Contributor

@allanrenucci allanrenucci Nov 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think you can write dependsOn(dottyCompiler, dottyLibrary, dottyBench)

.drone.yml Outdated
@@ -13,16 +13,15 @@ pipeline:
image: lampepfl/dotty:2017-10-20
commands:
- cp -R . /tmp/1/ && cd /tmp/1/
- ./project/scripts/sbt compile
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do ./project/scripts/sbt ";compile ;testAll" to avoid restarting sbt

smarter
smarter previously requested changes Nov 1, 2017
View reviewed changes
project/Build.scala Outdated
@@ -1132,8 +1138,7 @@ object Build {
def asDottyRoot(implicit mode: Mode): Project = project.withCommonSettings.
aggregate(`dotty-interfaces`, dottyLibrary, dottyCompiler, dottyDoc, dottySbtBridgeReference).
bootstrappedAggregate(`scala-library`, `scala-compiler`, `scala-reflect`, scalap, `dotty-language-server`).
dependsOn(dottyCompiler).
dependsOn(dottyLibrary).
dependsOn(dottyCompiler, dottyLibrary, dottyBench).
Copy link
Member

@smarter smarter Nov 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Please don't do this. sbt currently downloads the dotty artifact and it's dependencies, so everyone will download dotty-bench if you add it as a dependency

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add something like a noPublish setting to dottyBench so that is is not included in the dotty artefact?

Copy link
Member

Choose a reason for hiding this comment

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

If you don't publish it, it will still be listed as a dependency, and downloading dotty will fail when the dependency is not found.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same if we put it in aggregate and not dependsOn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove this part

@nicolasstucki nicolasstucki force-pushed the ensure-all-is-compiled-in-ci branch from 0c6a239 to 43f57ab Compare November 2, 2017 09:06
@nicolasstucki nicolasstucki dismissed stale reviews from smarter and allanrenucci November 2, 2017 09:07

The dotty root change was removed

allanrenucci
allanrenucci suggested changes Nov 2, 2017
View reviewed changes
Copy link
Contributor

@allanrenucci allanrenucci left a comment

Choose a reason for hiding this comment

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

This does work unfor

.drone.yml Outdated
@@ -13,16 +13,14 @@ pipeline:
image: lampepfl/dotty:2017-10-20
commands:
- cp -R . /tmp/1/ && cd /tmp/1/
- ./project/scripts/sbt testAll
- ./project/scripts/sbt ";dotty-bench/jmh:run 1 1 tests/run/arrays.scala"
- ./project/scripts/sbt compile testAll "dotty-bench/jmh:run 1 1 tests/pos/alias.scala"
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not work unfortunately. The project/script/sbt script expects one argument. Either use the old syntax or change the script

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, do we have to run the dotty-bench project? Can we just compile it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I remembered that I added it because at some point we had a runtime bug on that project that stayed undetected for a while.

.drone.yml Outdated
@@ -59,7 +57,7 @@ pipeline:
environment:
- NIGHTLYBUILD=yes
commands:
- ./project/scripts/sbtPublish ";dotty-bootstrapped/publishSigned ;sonatypeRelease"
- ./project/scripts/sbtPublish dotty-bootstrapped/publishSigned sonatypeRelease
Copy link
Contributor

Choose a reason for hiding this comment

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

project/script/sbtPublish also expects one argument. I am fine with changing it

@nicolasstucki nicolasstucki force-pushed the ensure-all-is-compiled-in-ci branch from 43f57ab to fdc8a58 Compare November 2, 2017 10:46
allanrenucci
allanrenucci approved these changes Nov 2, 2017
View reviewed changes
Copy link
Contributor

@allanrenucci allanrenucci left a comment

Choose a reason for hiding this comment

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

LGTM. Please squash your commits

.drone.yml Outdated

test_bootstrapped:
group: test
image: lampepfl/dotty:2017-10-20
commands:
- cp -R . /tmp/2/ && cd /tmp/2/
- ./project/scripts/sbt dotty-bootstrapped/testAll
- ./project/scripts/sbt ";dotty-bench-bootstrapped/jmh:run 1 1 tests/run/arrays.scala"
- ./project/scripts/sbt ";dotty-bootstrapped/testAll ;dotty-bench-bootstrapped/jmh:run 1 1 tests/pos/alias.scala"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add ;dotty-bootstrapped/compile here too

Copy link
Contributor

Choose a reason for hiding this comment

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

Won't dotty-bootstrapped/testAll do the compilation step already?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicolasstucki You said at some point that testAll didn't build DottyDoc, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not. Or at least it was missing some part of it.

@nicolasstucki nicolasstucki force-pushed the ensure-all-is-compiled-in-ci branch from 52bb727 to d9e5688 Compare November 2, 2017 14:57
@nicolasstucki nicolasstucki merged commit abd1e72 into scala:master Nov 3, 2017
@allanrenucci allanrenucci deleted the ensure-all-is-compiled-in-ci branch November 3, 2017 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter left review comments

@Duhemm Duhemm Duhemm left review comments

@allanrenucci allanrenucci allanrenucci approved these changes

Assignees

@allanrenucci allanrenucci

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nicolasstucki @smarter @Duhemm @allanrenucci