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

Build: Fail if any libs depend on non-core libs #29336

Merged
merged 5 commits into from
Apr 16, 2018

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Apr 2, 2018

Adds a precommit to every subprojects of :libs that fails the build
if there are any :libs dependencies.

I imagine it'd be possible to fail the build on startup if the
dependencies are unacceptable rather than use a precommit task but the
precommit task reports the error nicely and is easy to skip if for some
reason we want to temporarily skip it. So it felt like the right way to
go.

Since we now have three places where we resolve project substitutions
I've added dependencyToProject to project.ext in all projects. It
resolves both project style dependencies and "external" style (like
"org.elasticsearch:elasticsearch-core:${version}") dependencies to
Projects using the projectSubstitutions. I use this new function all
three places where resovle project substitutions.

Finally this pulls apply plugin: 'elasticsearch.build' out of
libs/*/build.gradle and into a subprojects clause in
libs/build.gradle. I do this entirely so that I can call
tasks.precommit.dependsOn checkDependencies without waiting for the
subprojects to be evaluated or worrying about whether or not they have
precommit set up in a normal way.

Adds a `precommit` to every subprojects of `:libs` that fails the build
if there are any `:libs` dependencies.

I imagine it'd be possible to fail the build on startup if the
dependencies are unacceptable rather than use a precommit task but the
precommit task reports the error nicely and is easy to skip if for some
reason we want to temporarily skip it. So it felt like the right way to
go.

Since we now have three places where we resolve project substitutions
I've added `dependencyToProject` to `project.ext` in all projects. It
resolves both `project` style dependencies and "external" style (like
"org.elasticsearch:elasticsearch-core:${version}") dependencies to
`Project`s using the `projectSubstitutions`. I use this new function all
three places where resovle project substitutions.

Finally this pulls `apply plugin: 'elasticsearch.build'` out of
`libs/*/build.gradle` and into a subprojects clause in
`libs/build.gradle`. I do this entirely so that I can call
`tasks.precommit.dependsOn checkDependencies` without waiting for the
subprojects to be evaluated or worrying about whether or not they have
`precommit` set up in a normal way.
@nik9000 nik9000 added review :Delivery/Build Build or test infrastructure v7.0.0 v6.3.0 labels Apr 2, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@rjernst
Copy link
Member

rjernst commented Apr 2, 2018

I don't think we should do this as a task. This should fail during configuration. We already do similar traversal inside BuildPlugin.configureConfigurations, where we disable transitive dependencies (and always force version conflict failure). I also think this is going to be a little more complicated as we need "groups" of libs that can have dependencies on each other (eg painless is going to have painless-compiler and painless-whitelist jars).

@jasontedor
Copy link
Member

I also think this is going to be a little more complicated as we need "groups" of libs that can have dependencies on each other (eg painless is going to have painless-compiler and painless-whitelist jars).

I think we can wait to cross that bridge when we get there.

@rjernst
Copy link
Member

rjernst commented Apr 2, 2018

I think we can wait to cross that bridge when we get there.

I'm in the middle of splitting the painless jars out right now.

@jasontedor
Copy link
Member

I'm in the middle of splitting the painless jars out right now.

That is, by my estimation, at least two if not three weeks away. Let us make the "group" change a follow-up to this PR, we can progress when the need is clearer.

@rjernst
Copy link
Member

rjernst commented Apr 6, 2018

That's fine, but my concern about the way this is implemented still stands. Doing it via a task is not right; it needs to be checked during configuration.

@jasontedor
Copy link
Member

That's fine, but my concern about the way this is implemented still stands. Doing it via a task is not right; it needs to be checked during configuration.

I have no objections to that, my comment was only about adding the "group" requirement on top of this specific PR.

@nik9000
Copy link
Member Author

nik9000 commented Apr 10, 2018

I don't think we should do this as a task. This should fail during configuration.

If I think "there are two ways to do this and either are just fine" and I basically flip a coin @rjernst almost always has good arguments against the result of my coin flip. Well, more again the "either are just fine" part of my reasoning, but still. I'll change it to fail on startup.

@nik9000
Copy link
Member Author

nik9000 commented Apr 12, 2018

I've switched this to run during configuration.

@nik9000
Copy link
Member Author

nik9000 commented Apr 12, 2018

@rjernst, would you like to take another look? I've switched this to run during configuration.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

This looks much better @nik9000, thanks! I left a few comments.


/*
* Subprojects may depend on the "core" lib but may not depend on any
* other Elasticsearch code. This keeps are dependencies simpler to think
Copy link
Member

Choose a reason for hiding this comment

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

other Elasticsearch code -> other lib

Also, I would reword the second sentence to "This keeps the dependency graph simple"

Copy link
Member Author

Choose a reason for hiding this comment

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

++

configurations.all { Configuration conf ->
dependencies.all { Dependency dep ->
if (project.name.equals('elasticsearch-nio')) {
System.err.println("ccc $conf.name $dep")
Copy link
Member

Choose a reason for hiding this comment

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

This looks like leftover debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll shoot it.

throw new InvalidUserDataException("projects in :libs "
+ "may not depend on other projects libs except "
+ ":libs:elasticsearch-core but "
+ "$project.path depends on $depProject.path")
Copy link
Member

Choose a reason for hiding this comment

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

I think you need brackets around these derefs? ${project.path} depends on ${depProject.path}

Copy link
Member Author

Choose a reason for hiding this comment

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

It isn't required but if you like it better I'm happy to do it that way.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is clearer that way.

@nik9000 nik9000 merged commit 69aabb7 into elastic:master Apr 16, 2018
nik9000 added a commit that referenced this pull request Apr 16, 2018
Fails the build if any subprojects of `:libs` have dependencies in `:libs`
except for `:libs:elasticsearch-core`.

Since we now have three places where we resolve project substitutions
I've added `dependencyToProject` to `project.ext` in all projects. It
resolves both `project` style dependencies and "external" style (like
"org.elasticsearch:elasticsearch-core:${version}") dependencies to
`Project`s using the `projectSubstitutions`. I use this new function all
three places where resovle project substitutions.

Finally this pulls `apply plugin: 'elasticsearch.build'` out of
`libs/*/build.gradle` and into a subprojects clause in
`libs/build.gradle`. I do this entirely so that I can call
`tasks.precommit.dependsOn checkDependencies` without waiting for the
subprojects to be evaluated or worrying about whether or not they have
`precommit` set up in a normal way.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 16, 2018
* master:
  Remove PipelineExecutionService#executeIndexRequest (elastic#29537)
  Fix overflow error in parsing of long geohashes (elastic#29418)
  Remove unused index.ttl.disable_purge setting (elastic#29527)
  FullClusterRestartIT.testRecovery should wait for all initializing shards
  Build: Fail if any libs depend on non-core libs (elastic#29336)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 17, 2018
* master: (96 commits)
  TEST: Mute testEnsureWeReconnect
  Mute full cluster restart test recovery test
  REST high-level client: add support for Indices Update Settings API [take 2] (elastic#29327)
  Plugins: Fix native controller confirmation for non-meta plugin (elastic#29434)
  Remove PipelineExecutionService#executeIndexRequest (elastic#29537)
  Fix overflow error in parsing of long geohashes (elastic#29418)
  Remove unused index.ttl.disable_purge setting (elastic#29527)
  FullClusterRestartIT.testRecovery should wait for all initializing shards
  Build: Fail if any libs depend on non-core libs (elastic#29336)
  [TEST] REST client request without leading '/' (elastic#29471)
  Using ObjectParser in UpdateRequest (elastic#29293)
  Prevent accidental changes of default values (elastic#29528)
  [Docs] Add definitions to glossary  (elastic#29127)
  Avoid self-deadlock in the translog (elastic#29520)
  Minor cleanup in NodeInfo.groovy
  Lazy configure build tasks that require older JDKs (elastic#29519)
  Simplify snapshot check in root build file
  Make NodeInfo#nodeVersion strongly-typed as Version (elastic#29515)
  Enable license header exclusions (elastic#29379)
  Use proper Java version for BWC builds (elastic#29493)
  ...
jpountz added a commit to jpountz/elasticsearch that referenced this pull request Apr 17, 2018
Currently this fails because the Eclipse configuration splits the main and test
folders into separate projects to avoid circular dependencies.

Relates elastic#29336
jpountz added a commit that referenced this pull request Apr 17, 2018
…29550)

Currently this fails because the Eclipse configuration splits the main and test
folders into separate projects to avoid circular dependencies.

Relates #29336
jpountz added a commit that referenced this pull request Apr 17, 2018
…29550)

Currently this fails because the Eclipse configuration splits the main and test
folders into separate projects to avoid circular dependencies.

Relates #29336
@colings86 colings86 removed the v7.0.0 label Feb 7, 2019
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v6.3.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants