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

Standardize build distribution internals on os/architecture #105842

Merged

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Feb 28, 2024

The build handles platform specific code which may be for arm or x86. Yet there are multiple ways to describe 64bit x86, and the build converts between the two in several places. This commit consolidates on the x64 nomenclature in most places, except where necessary (eg ML still uses x86_64).

relates #105715

The build handles platform specific code which may be for arm or x86.
Yet there are multiple ways to describe 64bit x86, and the build
converts between the two in several places. This commit consolidates on
the x64 nomenclature in most places, except where necessary (eg ML still
uses x86_64).

relates elastic#105715
@rjernst rjernst added :Delivery/Build Build or test infrastructure >refactoring labels Feb 28, 2024
@rjernst rjernst requested a review from a team as a code owner February 28, 2024 15:41
@elasticsearchmachine elasticsearchmachine added v8.14.0 Team:Delivery Meta label for Delivery team labels Feb 28, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@@ -65,42 +70,42 @@ CopySpec archiveFiles(CopySpec modulesFiles, String distributionType, String pla
distribution_archives {
integTestZip {
content {
archiveFiles(integTestModulesFiles, 'zip', null, 'x64', true)
Copy link
Contributor

@breskeby breskeby Feb 28, 2024

Choose a reason for hiding this comment

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

I'd personally prefer to keep it explicit here if it's a test distribution or not and keep that parameter in archiveFiles.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@breskeby breskeby 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 remark

if (os != null) {
if (architecture == 'x64') {
// ML platform dir uses the x86_64 nomenclature
architecture = 'x86_64'
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge fan of overriding a local argument like this. In this case we don't use it again later in this closure but we could and that would cause funny behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 1205c8e

@rjernst rjernst added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 28, 2024
@elasticsearchmachine elasticsearchmachine merged commit 067aba9 into elastic:main Feb 28, 2024
14 checks passed
@rjernst rjernst deleted the native/distribution_platform branch February 28, 2024 16:47
@mark-vieira
Copy link
Contributor

@rjernst can we backport this as well, at least to 8.13?

rjernst added a commit to rjernst/elasticsearch that referenced this pull request Feb 28, 2024
…105842)

The build handles platform specific code which may be for arm or x86.
Yet there are multiple ways to describe 64bit x86, and the build
converts between the two in several places. This commit consolidates on
the x64 nomenclature in most places, except where necessary (eg ML still
uses x86_64).

relates elastic#105715
elasticsearchmachine pushed a commit that referenced this pull request Feb 28, 2024
…#105846)

The build handles platform specific code which may be for arm or x86.
Yet there are multiple ways to describe 64bit x86, and the build
converts between the two in several places. This commit consolidates on
the x64 nomenclature in most places, except where necessary (eg ML still
uses x86_64).

relates #105715
idegtiarenko pushed a commit to idegtiarenko/elasticsearch that referenced this pull request Mar 4, 2024
…105842)

The build handles platform specific code which may be for arm or x86.
Yet there are multiple ways to describe 64bit x86, and the build
converts between the two in several places. This commit consolidates on
the x64 nomenclature in most places, except where necessary (eg ML still
uses x86_64).

relates elastic#105715
fang-xing-esql pushed a commit to fang-xing-esql/Elasticsearch that referenced this pull request Mar 8, 2024
…105842)

The build handles platform specific code which may be for arm or x86.
Yet there are multiple ways to describe 64bit x86, and the build
converts between the two in several places. This commit consolidates on
the x64 nomenclature in most places, except where necessary (eg ML still
uses x86_64).

relates elastic#105715
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Delivery/Build Build or test infrastructure >refactoring Team:Delivery Meta label for Delivery team v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants