Skip to content

Fix #5809: Run the community-build on every PR #5834

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

Merged
merged 3 commits into from
Feb 7, 2019

Conversation

OlivierBlanvillain
Copy link
Contributor

This PR is meant to replace the infrastructure previously setup in https://github.com/lampepfl/dotty-community-build. A new sbt project, community-build, is added to dotty's Build.scala and contains all the integration tests for the community build.

In case of failure the tests will output instructions on how to reproduce the error without JUnit. Hopefully these instructions are sufficient to have everyone fix regressions as they are introduced, or as a very last resort, update the code for the community build.

This should be the main improvement over the previous approach, where the community build maintenance was centralized (Allan did all the work) and asynchronous (it's now broken for more than a month...).

Another improvement is that a published local version of the sbt plugin is used when running the integration test, so breakage to sbt-dotty should be cough earlier than before.

Before merging, we should probably generate a new docker image for the CI where we cache all dependencies of the community projects.

@allanrenucci
Copy link
Contributor

allanrenucci commented Feb 1, 2019

What about we add .travis.yml file to the repo to run the community build on travis so that we keep the running time of normal tests low and we let travis take care of caching the community build dependencies?

@OlivierBlanvillain OlivierBlanvillain force-pushed the community-build branch 2 times, most recently from 982dd8c to 04e0dd5 Compare February 4, 2019 16:52
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

I also left some comments on dotty-staging/pdbp@604c06c since I'd like to see less modifications of the builds of each project.

@allanrenucci
Copy link
Contributor

Why does this rely on git submodules? The community build did not.

Adding this many submodules seems like a bad idea. They are a pain to work with and will make cloning the repo super slow...

@OlivierBlanvillain
Copy link
Contributor Author

OlivierBlanvillain commented Feb 4, 2019

@allanrenucci because we don't want to break the community build. Pointing to branches makes it very fragile, commits are more stable. It will also let us document potential changes required to community build to make a PR go tought, rather than having stuff happening silently over +10 repos.

Adding this many submodules seems like a bad idea. They are a pain to work with and will make cloning the repo super slow...

A follow up PR will remove the backend and scala2-lib submodules, which means we will get ride of the submodule requirement for using and testing dotty, and will only need submodules for the community build. In particular we can update drone and the docs to remove the --recursive.

@OlivierBlanvillain
Copy link
Contributor Author

@smarter I didn't notice that pdbp upstream uses the sbt-dotty plugin. I think in every other project the plugin was added to make it compile with dotty, so removing the plugin makes the build closer to it's upstream state. If you think it's worth the time I can do another pass to try minimizing the diff of every build...

@smarter
Copy link
Member

smarter commented Feb 4, 2019

Eventually, all upstream projects should compile with dotty, so we shouldn't have to modify their build (especially a modification that removes the plugin and therefore breaks the build).

@allanrenucci
Copy link
Contributor

allanrenucci commented Feb 4, 2019

because we don't want to break the community build. Pointing to branches makes it very fragile, commits are more stable

When I was maintaining the community build, I thought it was rather convenient. I would rebase the forks every now and then without needing to update the submodules. The community build never broke because a branch changed. We own the forks, we control branch changes. The forks also have their own CI, so as long as you PR changes to them, there is no reason it will break the community build.

If you think depending on a specific commit is desirable then I would add a new setting to the tests. E.g.

@Test def scalap = test(
  project = "scalap",
  command = "scalap/compile",
  commit = "ff707"
)
// We now run something like `git reset --hard <commit>`

Relying on submodule also makes it harder to add a new project to the community. It wouldn't help if we want external contribution to grow the community build.

@OlivierBlanvillain
Copy link
Contributor Author

OlivierBlanvillain commented Feb 4, 2019

Yes the setup was acceptable when its a single person maintaining it and it was not run on every PR, but now I really don't want to have PRs on Dotty being blocked because somebody fucked up something in the community build. Also I really dislike that we do random stuff like this without anyway to relate them to changes in Dotty. Submodules solve both problems.

If you think depending on a specific commit is desirable then I would add a new setting to the tests.

You are suggesting to have a file with a list of commit per project, this is exactly what submodules are!

Relying on submodule also makes it harder to add a new project to the community. It wouldn't help if we want external contribution to grow the community build.

It's literally git submodule add <url> + one entry in CommunityBuildTests.scala...

@OlivierBlanvillain OlivierBlanvillain force-pushed the community-build branch 2 times, most recently from 499ee95 to f72e41b Compare February 5, 2019 14:07
@OlivierBlanvillain
Copy link
Contributor Author

I'm not very happy with Travis, running time are very unpredictable. For instance, doing dotty-compiler-bootstrapped/publish-local alone (which is a prerequisite for testing projects in the community build) can take from ~9 min to ~16.5 min (these are the extremums among the few runs in this PR). I think I'm going to revert back to the original idea of doing it all in drone, with the downside of having to manually update the cache in the docker image...

@OlivierBlanvillain
Copy link
Contributor Author

With the cache setup sequentially running the entire full community build seams to be faster than test_bootstrapped, so I think it's better to stick with drone for now. @smarter Do you want to have a second look?

This commit is meant to replace the infrastructure previously setup in
https://github.com/lampepfl/dotty-community-build. A new sbt project,
community-build, is added to dotty's Build.scala and contains all the
integration tests for the community build.

In case of failure the tests will output instructions on how to
reproduce the error without JUnit. Hopefully these instructions are
sufficient to have everyone fix regressions as they are introduced,
or as a very last resort, update the code for the community build.

This should be the main improvement over the previous approach, where
the community build maintenance was centralized (Allan did all the work)
and asynchronous (it's now broken for more than a month...).

Another improvement is that a published local version of the sbt plugin
is used when running the integration test, so breakage to sbt-dotty
should be caught earlier than before.
@OlivierBlanvillain OlivierBlanvillain merged commit 732f7e2 into scala:master Feb 7, 2019
@Blaisorblade Blaisorblade deleted the community-build branch February 7, 2019 12:47
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.

3 participants