Skip to content

Add scala scala submodule. #2323

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 8 commits into from
May 5, 2017
Merged

Add scala scala submodule. #2323

merged 8 commits into from
May 5, 2017

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@DarkDimius
Copy link
Contributor

LGTM after tests pass

@smarter
Copy link
Member

smarter commented Apr 28, 2017

I would call it scala-library, not scala-scala

DarkDimius
DarkDimius reviewed Apr 28, 2017
View reviewed changes
compiler/test/dotty/tools/dotc/CompilationTests.scala
@@ -29,6 +29,7 @@ class CompilationTests extends ParallelTesting {

@Test def compilePos: Unit = {
compileList("compileStdLib", StdLibSources.whitelisted, scala2Mode.and("-migration", "-Yno-inline")) +
compileDir("../collection-strawman/src/main", defaultOptions) +
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe make it a separate test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We would loose the parallelism.

smarter
smarter reviewed Apr 28, 2017
View reviewed changes
project/Build.scala Outdated
val scalaScala = new File("scala-scala")
if (!scalaScala.exists()) {
val submodules = List(new File("scala-backend"), new File("scala-scala"), new File("collection-strawman"))
if (!submodules.forall(_.exists)) {
println(
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't use println in the sbt build, see http://www.scala-sbt.org/0.13/docs/Howto-Logging.html#Log+messages+in+a+task for the proper way to do it. This should probably be logged at the error level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a task.

Copy link
Member

Choose a reason for hiding this comment

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

But could it be one?

@smarter
Copy link
Member

smarter commented Apr 28, 2017

Please go through the documentation doc/doc/ to remove references to scala-scala.

@nicolasstucki nicolasstucki force-pushed the add-scala-scala-submodule branch 2 times, most recently from 6897c96 to b3f9a8e Compare April 28, 2017 13:22
@nicolasstucki
Copy link
Contributor Author

I noticed that if I set the scala-library submodule to the lampepfl/scala with branch sharing-backend the only test that fails is dotty.tools.dottydoc.TestWhitelistedCollections.arrayAndImmutableHasDocumentation. If we fix this we could remove use a single library/backend submodule. @DarkDimius @smarter what do you think about that?

@smarter
Copy link
Member

smarter commented Apr 28, 2017

@nicolasstucki You probably forgot to do git submodule update, there is no way that the library in sharing-backend compiles with dotty, it uses procedure syntax, etc.

@nicolasstucki nicolasstucki force-pushed the add-scala-scala-submodule branch from b3f9a8e to adb36ee Compare April 28, 2017 14:03
@nicolasstucki
Copy link
Contributor Author

@smarter indeed at first I forgot to do git submodule update and noticed that it passed all tests except TestWhitelistedCollections on lampepfl/scala with branch master. Then I explicitly change the branch to sharing-backend and saw the same.

All the tests on the scala-library use the scala2 flag. That is why it can handle procedure syntax, etc.

@nicolasstucki
Copy link
Contributor Author

@DarkDimius, could you check if it still LGTY. If this is merged it would simplify a bit the call graph branch tests.

@nicolasstucki nicolasstucki force-pushed the add-scala-scala-submodule branch 2 times, most recently from d108479 to 04eee0d Compare May 2, 2017 15:57
@nicolasstucki nicolasstucki force-pushed the add-scala-scala-submodule branch 2 times, most recently from bb192de to c0779ea Compare May 3, 2017 09:26
@nicolasstucki
Copy link
Contributor Author

@smarter, do you agree with the changes in the last commit?

@nicolasstucki nicolasstucki force-pushed the add-scala-scala-submodule branch from c0779ea to 5420a03 Compare May 4, 2017 11:32
@DarkDimius
Copy link
Contributor

LGTM from my side, didn't check sbt though.

@smarter smarter merged commit 62deba5 into scala:master May 5, 2017
@nicolasstucki nicolasstucki mentioned this pull request May 5, 2017
Closed
@allanrenucci allanrenucci deleted the add-scala-scala-submodule branch December 14, 2017 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter approved these changes

@DarkDimius DarkDimius DarkDimius approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nicolasstucki @DarkDimius @smarter