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] support building targets for platform and arch inputs #776

Merged
merged 1 commit into from
Oct 21, 2021

Conversation

kavilla
Copy link
Member

@kavilla kavilla commented Oct 21, 2021

Description

Adding platform and arch as a build argument to be passed to
the build target. This enables cross-platform cross-arch builds,
for example, we can build linux x64 on a linux ARM64 machine.

We would want to do this because some of the libraries that are
required for building (not for runtime) are not available for
certain operating systems.

Also updated the OpenSearch Dashboards Jenkinsfile to pass the
platform and arch.

Signed-off-by: Kawika Avilla kavilla414@gmail.com

Issues Resolved

#739

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2021

Codecov Report

Merging #776 (04533cb) into main (b2fe209) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #776      +/-   ##
==========================================
+ Coverage   91.29%   91.33%   +0.03%     
==========================================
  Files          75       75              
  Lines        1999     2019      +20     
==========================================
+ Hits         1825     1844      +19     
- Misses        174      175       +1     
Impacted Files Coverage Δ
src/build_workflow/builder.py 100.00% <ø> (ø)
src/assemble_workflow/bundle_recorder.py 100.00% <100.00%> (ø)
src/build_workflow/build_args.py 100.00% <100.00%> (ø)
src/build_workflow/build_recorder.py 100.00% <100.00%> (ø)
src/build_workflow/build_target.py 100.00% <100.00%> (ø)
src/run_build.py 85.00% <100.00%> (ø)
src/system/os.py 100.00% <100.00%> (ø)
src/manifests/input_manifest.py 95.91% <0.00%> (-1.38%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2fe209...04533cb. Read the comment docs.

@kavilla
Copy link
Member Author

kavilla commented Oct 21, 2021

This comment: #763 (comment)

You need defaults for these or it will be blank and break the existing OpenSearch build.

I actually added defaults to a branch:
#777

Please don't review that draft PR fully but just want to say if we absolutely wanted defaults we can go for it.

However, it would require to pass the arm64 in the OpenSearch Jenkinsfile, and it actually builds successfully without defaults. I ran through this in a pipeline of my own that doesn't publish and I got
Screen Shot 2021-10-20 at 7 50 28 PM

It successful because then it defaults to the OS plat and arch: https://github.com/opensearch-project/opensearch-build/blob/main/src/build_workflow/build_target.py#L36.

script { manifest = readYaml(file: 'artifacts/manifest.yml') }
def artifactPath = "${manifest.build.version}/${BUILD_NUMBER}/${manifest.build.architecture}";
def artifactPath = "${manifest.build.version}/${BUILD_NUMBER}/${manifest.build.platform}-${manifest.build.architecture}";
Copy link
Member Author

Choose a reason for hiding this comment

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

highlighting updated s3 path.

Copy link
Member

Choose a reason for hiding this comment

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

You've got a race with #765 which cleans this all up

SUPPORTED_PLATFORMS = [
"linux"
]
SUPPORTED_ARCHITECTURES = [
Copy link
Member Author

Choose a reason for hiding this comment

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

highlighting these proposed arches.

@kavilla kavilla requested review from dblock, peterzhuamazon, peternied and Tengda-He and removed request for dblock October 21, 2021 02:55
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This LGTM! Let's fix the list of architectures.

]
SUPPORTED_ARCHITECTURES = [
"x64",
"x86_64",
Copy link
Member

Choose a reason for hiding this comment

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

Are x64 and x86_64 a dup here? I never know which one we pass from where, I think the two here should be x64 and arm64, since this is what we return from current_architecture.

Adding `platform` and `arch` as a build argument to be passed to
the build target. This enables cross-platform cross-arch builds,
for example, we can build linux x64 on a linux ARM64 machine.

We would want to do this because some of the libraries that are
required for building (not for runtime) are not available for
certain operating systems.

Also updated the OpenSearch Dashboards Jenkinsfile to pass the
platform and arch.

Issue resolved:
opensearch-project#739

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
@kavilla kavilla requested a review from dblock October 21, 2021 21:00
script { manifest = readYaml(file: 'artifacts/manifest.yml') }
def artifactPath = "${manifest.build.version}/${BUILD_NUMBER}/${manifest.build.architecture}";
def artifactPath = "${manifest.build.version}/${BUILD_NUMBER}/${manifest.build.platform}-${manifest.build.architecture}";
Copy link
Member

Choose a reason for hiding this comment

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

You've got a race with #765 which cleans this all up

@peternied peternied merged commit 6ae52ca into opensearch-project:main Oct 21, 2021
@kavilla kavilla deleted the avillk/archarg branch October 22, 2021 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Dashboards build agent can't build on ARM64 due to chromium missing
4 participants