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

Access build tools resources #32201

Merged
merged 8 commits into from
Aug 10, 2018

Conversation

alpar-t
Copy link
Contributor

@alpar-t alpar-t commented Jul 19, 2018

The build plugin now adds a task to write out specific resources meant for consumption to the file sustem.

exportBuildResources {
    resource 'checkstyle_conf', "$buildDir/checkstyle.xml"
}

The task is not ran by default, the tasks that need the files should depend on it.
Will update existing uses in checkstyle and look for others in a subsequent PR.

Came out of a discussion following #31989.

@alpar-t alpar-t added the :Delivery/Build Build or test infrastructure label Jul 19, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@rjernst
Copy link
Member

rjernst commented Jul 23, 2018

@atorok Thanks for getting started on this. Accessing resources from build tools is something we do in many places in our build, but it has created a spaghetti network of implicit cross project dependencies, and formalizing these will be great for ensuring our gradle plugin continues to work externally.

I have 2 general comments so far:

  • I don't think we need to be so strict as to require every file accessible be explicitly defined in build-tools. Any build file can always do BuildPlugin.class.getResourcesAsStream, so having the extra work to "whitelist" the files allowed just makes getting to new files added cumbersome.
  • How do you envision tests actually using this task? It looks like a number of steps would be necessary, including creating a local path, specifying the file to copy in this new task, adding a task dependency, and using the local file path in the task consuming the file. Is there a way we can reduce the amount of boilerplate necessary?

@alpar-t
Copy link
Contributor Author

alpar-t commented Jul 24, 2018

I wasn't 100% sure on the white-listing either, will remove it.

We could add conventions to reduce the boilerplate.
If we have a pre-configured output directory and always export there to the same path as provided, then one just has to configure the newly added task to export a specific file and declare a dependency on it.
If we want to avoid specifying the file to export, we could create a list of resources at build time and include it into the resources, so that the task would know what all the resources are and always export all of them, but not sure we should do that. We might be able to go away with the explicit dependency using lazy configuation I haven't used this one before, but I will take a look at how we can use it.
If we could have the newly created task expose providers for specific files so that by writing something like task example { doLast { println "using: " + buildTools.resource('foo/bar').get() } } would configure the export task to export the specified file and imply a dependency to it from the example task, it would be very nice to use.

@alpar-t
Copy link
Contributor Author

alpar-t commented Jul 25, 2018

I found gradle/gradle#3811 which has some more insight into why some of the lazy configuration doesn't work.

@alpar-t
Copy link
Contributor Author

alpar-t commented Jul 25, 2018

@elasticmachine test this please

1 similar comment
@alpar-t
Copy link
Contributor Author

alpar-t commented Jul 25, 2018

@elasticmachine test this please

@alpar-t
Copy link
Contributor Author

alpar-t commented Aug 6, 2018

@rjernst this is ready for another review
@elasticmachine test this please

@alpar-t alpar-t requested a review from rjernst August 7, 2018 13:14

exportBuildResources {
resource 'checkstyle.xml'
resource 'minimumCompilerVersion'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use other files, e.x. forbidden APIs to avoid confusion.

@alpar-t
Copy link
Contributor Author

alpar-t commented Aug 8, 2018

Rename task to buildResources and method to use

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.

LGTM. One suggestion: change "exportBuildResources" to "buildResources" and then the method to "use", so it looks more like a normal DSL.

@alpar-t
Copy link
Contributor Author

alpar-t commented Aug 10, 2018

it's buildResources.take as use is a keyword in Groovy. Let me know if you have an idea for a better verb.

@alpar-t alpar-t merged commit ef21ab8 into elastic:master Aug 10, 2018
@alpar-t alpar-t deleted the access-build-tools-resources branch August 10, 2018 15:03
alpar-t added a commit that referenced this pull request Aug 10, 2018
Add a buildResources 

buildResources.take('resource') writes the resources out to the filesystem so the paths can be sent int other tools.
@rjernst
Copy link
Member

rjernst commented Aug 10, 2018

@atorok I don't think take is a good term here. It has both negative connotations and doesn't accurately portray what the buildResources task is doing (it isn't taking resources away). We previously discussed having the task containing "copy" in the name, but you did not want it confused with the gradle copy task. However, I think the inner part of the dsl here could be copy? That is technically what it is doing, copying a resource for local consumption.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 13, 2018
…listeners

* elastic/master: (58 commits)
  [ML] Partition-wise maximum scores (elastic#32748)
  [DOCS] XContentBuilder#bytes method removed, using BytesReference.bytes(docBuilder) (elastic#32771)
  HLRC: migration get assistance API (elastic#32744)
  Add a task to run forbiddenapis using cli (elastic#32076)
  [Kerberos] Add debug log statement for exceptions (elastic#32663)
  Make x-pack core pull transport-nio (elastic#32757)
  Painless: Clean Up Whitelist Names (elastic#32791)
  Cat apis: Fix index creation time to use strict date format (elastic#32510)
  Clear Job#finished_time when it is opened (elastic#32605) (elastic#32755)
  Test: Only sniff host metadata for node_selectors (elastic#32750)
  Update scripted metric docs to use `state` variable (elastic#32695)
  Painless: Clean up PainlessCast (elastic#32754)
  [TEST] Certificate NONE not allowed in FIPS JVM (elastic#32753)
  [ML] Refactor ProcessCtrl into Autodetect and Normalizer builders (elastic#32720)
  Access build tools resources (elastic#32201)
  Tests: Disable rolling upgrade tests with system key on fips JVM (elastic#32775)
  HLRC: Ban LoggingDeprecationHandler (elastic#32756)
  Fix test reproducability in AbstractBuilderTestCase setup (elastic#32403)
  Only require java<version>_home env var if needed
  Tests: Muted ScriptDocValuesDatesTests.testJodaTimeBwc
  ...
@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.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants