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.

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

Docs: Update CONTRIBUTING.md with shortcut command for assembling only the tar distribution #35276

Merged
merged 4 commits into from
Nov 7, 2018
Merged

Docs: Update CONTRIBUTING.md with shortcut command for assembling only the tar distribution #35276

merged 4 commits into from
Nov 7, 2018

Conversation

cjcenizal
Copy link
Contributor

This will help people assemble distributions without running into a problem I ran into when running the normal assemble command, in which buildBwcVersion failed because I lacked old Java versions:

> Task :distribution:bwc:bugfix:buildBwcVersion FAILED

 [6.4.3] ERROR: JAVA_HOME is set to an invalid directory: null
 [6.4.3]
 [6.4.3] Please set the JAVA_HOME variable in your environment to match the
 [6.4.3] location of your Java installation.

@cjcenizal
Docs: Update CONTRIBUTING.md with shortcut command for assembling onl…
a1c5325 
…y the tar distribution, as opposed to all distributable artifacts.
@cjcenizal cjcenizal added >docs General docs changes :Delivery/Build Build or test infrastructure v7.0.0 v6.6.0 labels Nov 5, 2018
@cjcenizal cjcenizal requested a review from nik9000 November 5, 2018 23:00
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

jasontedor
jasontedor reviewed Nov 6, 2018
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

I left a few nits, I'll defer to @nik9000 for overall approval.

CONTRIBUTING.md Outdated
@@ -196,7 +196,7 @@ the settings window and/or restart IntelliJ to see your changes take effect.

### Creating A Distribution

To create a distribution from the source, simply run:
To create all distributable artifacts from the source, simply run:
Copy link
Member

Choose a reason for hiding this comment

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

all distributable artifacts -> all build artifacts

CONTRIBUTING.md Outdated
@@ -209,6 +209,13 @@ The package distributions (Debian and RPM) can be found under:
The archive distributions (tar and zip) can be found under:
`./distribution/archives/(tar|zip)/build/distributions/`

Note that this assembles every distributable artifact (e.g. plugins, javadocs) as well as
Copy link
Member

Choose a reason for hiding this comment

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

distributable -> build
(e.g. plugins, javadocs) -> (e.g., plugins and Javadocs)
javadocs -> Javadocs

@jasontedor
Copy link
Member

Thanks for picking this up @cjcenizal!

@nik9000
Copy link
Member

nik9000 commented Nov 6, 2018 via email

@cjcenizal
Copy link
Contributor Author

Thanks @jasontedor and @nik9000! I've addressed your feedback. Could you take another look? I've also added a note about the --parallel flag.

jasontedor
jasontedor reviewed Nov 6, 2018
View reviewed changes
CONTRIBUTING.md
You can also build the artifacts and distributions in parallel:

```sh
./gradlew assemble --parallel
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 that we can leave this paragraph out and put the --parallel on the two build commands (tar assemble and full assemble).

Copy link
Contributor Author

@cjcenizal cjcenizal Nov 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Is --parallel needed/useful when building just the tar?

Copy link
Member

Choose a reason for hiding this comment

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

It does make the build process faster, since pieces of compilation can occur as the dependencies are ready.

Copy link
Member

Choose a reason for hiding this comment

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

And it applies to both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks!

jasontedor
jasontedor approved these changes Nov 7, 2018
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@cjcenizal
Copy link
Contributor Author

@jasontedor Do I need to get this PR to pass CI?

@nik9000 I'll merge once I get your approval.

@nik9000
Copy link
Member

nik9000 commented Nov 7, 2018 via email

@nik9000
Copy link
Member

nik9000 commented Nov 7, 2018 via email

@cjcenizal cjcenizal merged commit 1614107 into elastic:master Nov 7, 2018
@cjcenizal cjcenizal deleted the docs/assemble-distribution-only branch November 7, 2018 01:22
@cjcenizal
Copy link
Contributor Author

Should I backport this to 6.x?

@jasontedor
Copy link
Member

jasontedor commented Nov 7, 2018 •
edited
Loading

Should I backport this to 6.x?

I expect most folks don't look at contributing docs in the non-master branch, but I do like when we keep our development branches up to date. So I would be good with cherry-picking this back to 6.x.

cjcenizal added a commit that referenced this pull request Nov 8, 2018
@cjcenizal
Docs: Update CONTRIBUTING.md with shortcut command for assembling onl…
efbc2df 
…y the tar distribution (#35276)

* Add --parallel flag.
pgomulka pushed a commit to pgomulka/elasticsearch that referenced this pull request Nov 13, 2018
@cjcenizal @pgomulka
Docs: Update CONTRIBUTING.md with shortcut command for assembling onl…
3ed09e3 
…y the tar distribution (elastic#35276)

* Add --parallel flag.
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels 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
Reviewers

@jasontedor jasontedor jasontedor approved these changes

@nik9000 nik9000 Awaiting requested review from nik9000

Assignees
No one assigned
Labels
:Delivery/Build Build or test infrastructure >docs General docs changes Team:Delivery Meta label for Delivery team v6.6.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cjcenizal @elasticmachine @jasontedor @nik9000 @mark-vieira @jimczi