Skip to content

Add gradle task to build platform specific distro #56003

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 5 commits into from
May 12, 2020
Merged

Add gradle task to build platform specific distro #56003

merged 5 commits into from
May 12, 2020

Conversation

breskeby
Copy link
Contributor

@breskeby breskeby commented Apr 30, 2020

This adds the gradle task ':installLocalDistribution' that installs a distribution locally matching the local architecture.

Closes #53682

@breskeby breskeby added >enhancement :Delivery/Build Build or test infrastructure labels Apr 30, 2020
@breskeby breskeby requested a review from mark-vieira April 30, 2020 08:42
@breskeby breskeby self-assigned this Apr 30, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Build)

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Apr 30, 2020
Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

I think we want to leverage our DistributionDownloadPlugin here. The plugin has all sort of smarts in it already to figure out how to resolve a requested distribution type to a project dependency or download. So I'd recommend we go that route so we don't have to duplicate the logic of which project to delegate to depending on the current os (that lives in the plugin already). Also, we'll probably want this task to extract the distro to some standard location. That way we can easily document that you can run this task, then look in /extracted/dir to find our distro. As it is, the build artifact is going to live in the build dir for the specific platform project, which is kind of confusing.

So let's do this:

  1. Create a new script plugin for this and apply to the root project so we don't bloat the root build.gradle any further.
  2. Apply the elasticsearch.distribution-download plugin.
  3. Configure an ElasticsearchDistribution for the current platform, with type = 'archive' and flavor = 'default'.
  4. Wire that distribution up to a copy task that copy the extracted distro to a known location see ElasticsearchDistribution.getExtracted()`.

@breskeby
Copy link
Contributor Author

breskeby commented Apr 30, 2020 via email

@mark-vieira
Copy link
Contributor

Just to get a better understanding, why would we package and unpackage instead of just having a plain copy task that might share the same copy spec with the distro archive tasks?

That's a fantastic question 😉 . Mainly because we just simply haven't optimized for that yet but it is very much worth doing as tar/untar adds considerable overhead. Right now we are basically just doing this all via artifact dependencies, so we grab the archive artifact, then unpack it. It'll need some work to make it more efficient.

@breskeby breskeby marked this pull request as draft May 7, 2020 10:24
breskeby added 2 commits May 7, 2020 14:07
This adds the gradle task ':assemblePlatformArchive' that depends on the
platform specific distro archive tasks and allows using the same task for
building only the archive according to the local platform.

This delegates to

   :distribution:archives:windows-zip:assemble on windows
   :distribution:archives:linux-tar:assemble on linux
   :distribution:archives:darwin-tar:assemble on osx

Closes #53682
- reuse distribution download plugin
- rename task to installLocalDistribution
@breskeby breskeby marked this pull request as ready for review May 11, 2020 12:26
@breskeby
Copy link
Contributor Author

@mark-vieira I applied your feedback

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@rjernst want to contribute to some bike-shedding about naming here 😉

}
}

tasks.register('installLocalDistribution', Copy) {
Copy link
Member

Choose a reason for hiding this comment

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

perhaps we could make this easier to type as just localDistro? Should we also use Sync instead of Copy? Can't older files that were removed be left in place with Copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

localDistro it is. And switched to use Sync

tasks.register('installLocalDistribution', Copy) {
from(elasticsearch_distributions.local.extracted)
into("build/distribution/local")
}
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 we should add a println that states the location of the expanded distribution

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call, let's add a user friendly doLast which prints something like Elasticsearch distribution installed to {location}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

*/

/**
* This script sets up a local distribution including.
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be some words missing here.

Copy link
Contributor Author

@breskeby breskeby May 11, 2020

Choose a reason for hiding this comment

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

fixed the comment

breskeby added 2 commits May 11, 2020 22:17
- rename task name to localDistro
- use Sync instead of Copy task
- add user friendly output to point user to right directory
@breskeby
Copy link
Contributor Author

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

breskeby added a commit to breskeby/elasticsearch that referenced this pull request May 12, 2020
* Add gradle task 'localDistro' to build platform specific distro
* reuse distribution download plugin

Closes elastic#53682
breskeby added a commit to breskeby/elasticsearch that referenced this pull request May 12, 2020
* Add gradle task 'localDistro' to build platform specific distro
* reuse distribution download plugin

Closes elastic#53682
breskeby added a commit to breskeby/elasticsearch that referenced this pull request May 12, 2020
* Add gradle task 'localDistro' to build platform specific distro
* reuse distribution download plugin

Closes elastic#53682
breskeby added a commit that referenced this pull request May 12, 2020
breskeby added a commit that referenced this pull request May 12, 2020
@mark-vieira
Copy link
Contributor

@breskeby FYI, I added the v7.9.0 label, corresponding to the 7.x branch.

breskeby added a commit that referenced this pull request May 13, 2020
@mark-vieira mark-vieira added Team:Delivery Meta label for Delivery team and removed Team:Core/Infra Meta label for core/infra team labels Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add gradle task to build platform-specific distribution only
6 participants