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

Refactoring CI regarding ARM binary publish #2136

Merged
merged 7 commits into from
Feb 7, 2022
Merged

Refactoring CI regarding ARM binary publish #2136

merged 7 commits into from
Feb 7, 2022

Conversation

curquiza
Copy link
Member

@curquiza curquiza commented Feb 2, 2022

Fixes #1909

  • Remove CI file to publish aarch64 binary and put the logic into publish-binary.yml
  • Remove the job to publish armv8 binary
  • Fix download-latest script accordingly
  • Adapt dowload-latest with the specific case of the MacOS m1

@curquiza curquiza added this to the v0.26.0 milestone Feb 2, 2022
@curquiza curquiza marked this pull request as draft February 2, 2022 18:29
@curquiza curquiza marked this pull request as ready for review February 2, 2022 18:34
## Environment variable is not passed using env:
## LD gold won't work with MUSL
# env:
# JEMALLOC_SYS_WITH_LG_PAGE: 16
Copy link
Member

Choose a reason for hiding this comment

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

Removing that line for the ARM build won’t be a problem?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which lines? These lines were already commented in the other file

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Whoops, it doesn't work on my Macbook Pro M1.
Capture d’écran 2022-02-07 à 13 23 20

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

I am not sure but I would like that first command that returns something different than zero should make the script abort or at least we should check that curl doesn't return something different than 0. It would help in not having Not Found in the final meilisearch file as it can be dangerous, we chmod u+x it and a simple bash script can be returned by a middleware or something... We should abort before chmoding the file.

download-latest.sh Outdated Show resolved Hide resolved
Co-authored-by: Clément Renault <clement@meilisearch.com>
@meilisearch meilisearch deleted a comment from meili-bot Feb 7, 2022
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Ho and BTW the download binary is not the aarch64 one when I use the script from my MacBook Pro M1, it is the x86_64 one and uses rosetta to run the binary.

$ file meilisearch
meilisearch: Mach-O 64-bit executable x86_64

@curquiza
Copy link
Member Author

curquiza commented Feb 7, 2022

Ho and BTW the download binary is not the aarch64 one when I use the script from my MacBook Pro M1, it is the x86_64 one and uses rosetta to run the binary.

This is expected, when using a M1, the downloaded binary should be x86_64. Maybe I'm wrong, but @tamo confirmed the aarch64 binary did not work on M1. Because, if aarch64 works on M1, I can make the script simpler. Can you test aarch64 binary on your mac @Kerollmops?

@Kerollmops
Copy link
Member

Indeed, aarch64 can't work on macOS as it is an ELF binary file, not a Mach-O one. If running the x86_64 binary on the M1 is expected then the only thing that I don't like is that we do not stop the script on the first encountered error.

Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Thank you. I'm validating this!

Comment on lines -49 to -52
arch: aarch64 # aka ARMv8
distro: ubuntu18.04
env: |
JEMALLOC_SYS_WITH_LG_PAGE: 16
Copy link
Member

Choose a reason for hiding this comment

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

@curquiza

Which lines? These lines were already commented in the other file

It was not commented here, for the arm binary

Copy link
Member Author

Choose a reason for hiding this comment

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

This CI is the old one to generate armv8 binary.
In the new CI (to generate aarch64 binary), line 87, we can see the env variable is still present if I'm correct

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes sorry I missed it my bad!

Comment on lines -49 to -52
arch: aarch64 # aka ARMv8
distro: ubuntu18.04
env: |
JEMALLOC_SYS_WITH_LG_PAGE: 16
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes sorry I missed it my bad!

@curquiza
Copy link
Member Author

curquiza commented Feb 7, 2022

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 7, 2022

@bors bors bot merged commit 752a0e1 into main Feb 7, 2022
@bors bors bot deleted the refacto-arm-ci branch February 7, 2022 16:22
@curquiza curquiza added the v0.26.0 PRs/issues solved in v0.26.0 label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v0.26.0 PRs/issues solved in v0.26.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ARMv8] About removing the old ARMv8 workflow and naming for the binary
4 participants