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

feat: darwin-arm64 #1116

Closed
wants to merge 11 commits into from
Closed

feat: darwin-arm64 #1116

wants to merge 11 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented May 8, 2021

can build on apple silicon if you add macos-amd64 to this.  Also need to tweak the script some, but this version bump is just a good idea.
@faddat faddat changed the title pkg version bump darwin-arm64 May 8, 2021
@faddat faddat changed the title darwin-arm64 feat: darwin-arm64 May 8, 2021
Copy link
Member

@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

Hey @faddat and @mangas (#1118)!

We recently decided to temporarily rm ARM support by another PR (not merged to develop yet) because we don't have a solid way of producing ARM binaries in a regular developer machine (amd64).

I think updating AMD binaries and leaving ARM ones out-of-date (because we can't produce them) seems unconsistent and will likely lead to bugs because they'll be pointing to different versions of nodetime.

I suggest to discover another way to produce these binaries for both architectures that can be done at all of our machines. We can also utilize Docker for this.

For anyone who's willing to investigate, there is this buildx feature of Docker, maybe it can be a solution. Please see:

What do you think?

@mangas
Copy link

mangas commented May 10, 2021

@ilgooz @faddat

What about pre compiling the binaries and making them available to the cache this way?

There are couple of managed services that would also enable building on m1. I think it would be a shame to not support this just because it can't be built on the developer machine, which btw could be a security flaw since the binaries can be built locally and are not checked against any known checksums as far as I can tell.

Could be interesting to explore moving this generation to the CICD side and that would also resolve the problem?

@ilgooz
Copy link
Member

ilgooz commented May 10, 2021

Uh, I just noticed that 5.1.0 version of pkg also started to support getting the ARM binary. This is super cool but doesn't work on my 20.04 LTS Ubuntu machine.

I recevived a bunch of Failed to make bytecode node14-arm64 for file ... from pkg, even if I disable v8 bytecode compilation feature because this time I start receiving Error! --no-bytecode and no source breaks final executable. Seems like someone else also has experienced with the same problem here and one reported that macOS doesn't have this problem at all.

Do you guys receive any errors or warnings with/without --no-bytecode specified from pkg when building for ARM on macOS?

@mangas we are not accepting pre-compiled nodetime binaries from outside collobrators for security reasons but I strongly agree that moving this part to CI for complete transparency is the way to go.

We can eighter have a CI task that only compares checksums and does not change the source code of Starport or changes Starport's source code by generating these binaries. I personally prefer the first option because it's simpler and eliminates the need of creating an automated PR from the GH workflow and the review process involved by that.

But if we cannot solve the errors I added above -which seems not happening on macOS according to the GH issue on pkg I linked-, we still need to go with option 2 and configure the GH workflow to run on a macOS machine.

@faddat
Copy link
Contributor Author

faddat commented May 10, 2021

I think that removing arm support, even temporarily, would be a pretty bad idea.

EG: I would absolutely be forced to maintain a version that includes ARM support. I prefer to contribute to this Starport, AND if we are doing things right, this really isn't a question we should have (the users system architecture)

One option is indeed docker buildx (I use it extensively), but those builds are going to take an incredibly long time, each time, because you would be compiling node.js using QEMU.

Pain and wasted time would surely occur using buildx.

I guess if I were going to question something and yeah, welcome to hell, the thing that I might question is actually the practice of embedding node.js altogether and instead what we might want to do is just support the very latest LTS, so for example, currently that would be node.js 16...

The same way that we currently support only the latest release of golang, we could say that the standard is "the latest LTS release of node.js"

All of this gets into what's actually the most common issue that I have seen users have with this product, and probably the most common issue in the world of the Go programming language:

Development environment.

Go needs a https://rustup.sh like yesterday. Obviously this is out of scope but I figure it's worth mentioning.

The number of times that I have had the conversation about oh by the way you need to export such and such and you should make a folder called go under your home folder and you should add those to your bash profile oh and by the way a bash profile is blah blah blah blah blah blah blah blah blah blah.....

Is really going up dramatically, to the point that I can say with confidence that it's a significant user experience issue for Starport itself.

Options to retain arm support for Linux and mac

  • Tendermint compiles the binaries and hosts them in IPFS somewhere
  • Use Docker buildx
  • Make a cross compile system

reasons to drop arm support

  • CosmWasm 😭

At one point I dug into the arm support issues surrounding cosmwasm and it's quite fair to say that I quickly became overwhelmed-- I figure that it's possible but I also figure that it's a solid month's work.... Additionally, no guarantees on that month's work. So, in regard to all my big plans to run validators on Raspberry Pi's, well those plans still work but they now must exclude every single chain that supports CosmWasm.

@ilgooz
Copy link
Member

ilgooz commented May 10, 2021

Hey @faddat! Did you have chance to read my comment above? In the v4.x version of pkg (uses v2.6 of pkg-fetch) it was lacking to support ARM binaries and I thought it's still the case for v5.x untill I manually check it's package.json and find out that it uses v3.0 version of pkg-fetch which includes the ARM binares.

After seen this and now knowing that pkg does not depend on the underlying architecture for any other reason, this already solves our problem with ARM binaries.

The only thing we need (a) to figure out is how to solve the error lines I mentioned in my comment regarding to Ubuntu machines or (b) we can create a special Dockerfile for creating nodetime binaries that has all the bits required for pkg to operate otherwise (c) create CI workflow runs on macOS to produce these binaries and create a PR on Starport with them.

Any of a, b, c would work but it would be nicer if we can try to solve this first with option b so, this could work in all of our machines. And then we can create a CI workflow to compare checksums as @mangas suggested.

I think we're pretty close to finding a solution and also we're not tight on the time for an implementation because the next release of Starport will still keep the ARM support.

@ilgooz
Copy link
Member

ilgooz commented May 10, 2021

@faddat btw, Starport no longer depends on proper Go installation with all that exports and stuff after #1012. 😃

This will be shipped in the next release.

@mangas
Copy link

mangas commented May 10, 2021

Uh, I just noticed that 5.1.0 version of pkg also started to support getting the ARM binary. This is super cool but doesn't work on my 20.04 LTS Ubuntu machine.

I recevived a bunch of Failed to make bytecode node14-arm64 for file ... from pkg, even if I disable v8 bytecode compilation feature because this time I start receiving Error! --no-bytecode and no source breaks final executable. Seems like someone else also has experienced with the same problem here and one reported that macOS doesn't have this problem at all.

Do you guys receive any errors or warnings with/without --no-bytecode specified from pkg when building for ARM on macOS?

@mangas we are not accepting pre-compiled nodetime binaries from outside collobrators for security reasons but I strongly agree that moving this part to CI for complete transparency is the way to go.

We can eighter have a CI task that only compares checksums and does not change the source code of Starport or changes Starport's source code by generating these binaries. I personally prefer the first option because it's simpler and eliminates the need of creating an automated PR from the GH workflow and the review process involved by that.

But if we cannot solve the errors I added above -which seems not happening on macOS according to the GH issue on pkg I linked-, we still need to go with option 2 and configure the GH workflow to run on a macOS machine.

@ilgooz I think the errors you got were cause by the fact that the binary version is not available pre-compiled. See the link I provided above. The solution to this would be that you could rent an m1 and add the pre-built version to your local cache. This could be added as part of the process I think. You can use the one I shared on my PR as an example and see if it passes your build. It should not trigger the rebuild. For me it only triggered everything the first time and then just used the local artifacts so I think the options are either provide these from outside the local build and download them.

The arm64 builds are now supported but probably for the same reason, pkg doesn't have a prebuilt package for that arch :(

@ilgooz
Copy link
Member

ilgooz commented May 10, 2021

@mangas are you sure that pre compiled binaries aren't provided by pkg with v5.x? Because I can confirm that it downloads the arm binary. Can you rm -rf ~/.pkg-cache/ and run the gen-nodetime script again to check?

@faddat
Copy link
Contributor Author

faddat commented May 10, 2021

The pre-compiled binaries are working for everything except m1

@mangas
Copy link

mangas commented May 10, 2021

Yep, I'm sure, it's completely different when you build it, see here

@faddat
Copy link
Contributor Author

faddat commented May 20, 2021

Hey, I am not following this repo as closely as I'd like but I want to suggest a way to put this issue:

#1110

to bed.

Probably the approach that I used here, isn't the best one. Also I just realized that coding it will be faster than describing. One caveat:

ignite/web#106

Merging this would allow us to be 100% latest-node-LTS.

@ilgooz Yep, I am familiar with buildx and multiplatform manifests (intimately so) but not certain how it applies here.

@mangas probably has the right solution.

I'll shortly submit a PR that removes the binaries from the repository altogether, and expects that they will be fetched at build time. Might affect the Makefile.

wdyt?

@faddat
Copy link
Contributor Author

faddat commented May 20, 2021

Completed:

  • Makefile calls gen-nodetime in pre-build
  • binaries removed from repository
  • Automatically downloads node-js 14.17.1
  • Currently depends on the master branch of pkg instead of a release, so that it uses pkg-fetch 3.1.0

@ilgooz
Copy link
Member

ilgooz commented May 20, 2021

Thanks @faddat, do you see the same logs on your machine when you run the gen-nodetime script?

                                                                                                  K��J��[@��T�c@�!�6�H��C���������������
                                                                                                                                        K����J��[@��T�c@�!�%�H��K@����T�S@�!��H��������������
                                                                                                                                                                                             ��������G�����V����[@��T�c@�!�
                �H��F��T�C@�!��H����SA��[B��cC��kD��sE��{ͨ�_��{�����
                                                                   ��I������
                                                                            @��{¨�_��{�����������S���������������������3��@��Ñ�T��!���H��SA��{Ĩ�_��{���������
                                                                                                                                                             ���������J���`B���`�@A��T$A�d�@�`�c
� @�`�"|���?@9�
               @��{¨�_��{�����S���@���`�!�������K�����<�	T������(�J�`��@�`
��@���T�@99������˖K�a@��@�`�?h 8�SA��@��{Ĩ�_֠��@��C��{����`��������
  ����9C
        @�B�D�ch`8#�8�������������
                                  @��{Ĩ�_��{����������
                                                      ����9B�C�� �8�������������
��                                                                              @��{Ĩ�_��{��b��@���᫑B�
  ����9
       @��D�chb8#�8�������������
                                @��{è�_��{��@���᫑�
                                                  �����C�B�"�8�������������
��                                                                         @��{è�_��{����`��������
  ����9C
        @�B�D�ch`8#�8������z������
                                  @��{Ĩ�_��{����������
                                                      ����9B�C�� �8������h������
��                                                                              @��{Ĩ�_��{���*`����ᯑ�
  ����9C
        @�B�D�ch`8#�8������T������
                                  @��{è�_��{���*��ᯑ�
                                                    ����9B�C�� �8������B������
�ᯑ�                                                                           @��{è�_��{��|@�`�����
   ����9C
         @�B�D�ch`8#�8������.������
                                   @��{è�_��{��|@���ᯑ�
                                                      ����9B�C�� �8������������
                                                                               @��{è�_��{�����c��� @�@��S�����[��C���!8@��C@� ?��������! �"��ւ�����������������!`����[@�����x84���A���#�!: File name too long
/home/ilker/.pkg-cache/v3.1/fetched-v14.17.0-linuxstatic-arm64: 28: �G�@�r����[@����T�c@�!���H��K@����T�S@�!���H��: not found
/home/ilker/.pkg-cache/v3.1/fetched-v14.17.0-linuxstatic-arm64: 1: �̻���y8�4������[@�G�@�: not found
/home/ilker/.pkg-cache/v3.1/fetched-v14.17.0-linuxstatic-arm64: 1:T�Tbq: not found
/home/ilker/.pkg-cache/v3.1/fetched-v14.17.0-linuxstatic-arm64: 2: T�T�q�T�C��*�����������������
                                                                                                ���q: not found
/home/ilker/.pkg-cache/v3.1/fetched-v14.17.0-linuxstatic-arm64: 28: @����T�C@�!���H��+@����T�3@�!���H��SA��[B��cC��{ͨ�_��{�����c���@��S�����[��C���!8@��@��#�: not found
/home/ilker/.pkg-cache/v3.1/fetched-v14.17.0-linuxstatic-arm64: 30: Syntax error: ")" unexpected
> Warning Failed to make bytecode node14-arm64 for file /snapshot/gen-nodetime/node_modules/pbkdf2/lib/default-encoding.js

@faddat
Copy link
Contributor Author

faddat commented May 20, 2021

Let me double check, but certainly don't recall seeing that.

PS: recommended invocation of gen-nodetime is now:

make install

@faddat
Copy link
Contributor Author

faddat commented May 20, 2021

@ilgooz

Yes, yes I do, when building in gitpods. It looks like it is printing a binary to the terminal.

@ilgooz
Copy link
Member

ilgooz commented May 20, 2021

Do you have an m1 machine, how about there?

@faddat
Copy link
Contributor Author

faddat commented May 20, 2021

I have an m1. Currently on an arch linux machine. Pulling out the m1 now; issue was not present when I pushed the latest commits.

Found it.

Will always refer to the amd64's as x64 and that should resolve it.

@faddat faddat marked this pull request as draft May 21, 2021 07:06
@faddat
Copy link
Contributor Author

faddat commented May 24, 2021

@barriebyron this works on my M1 machine now.

How about you?

@faddat faddat marked this pull request as ready for review May 24, 2021 08:13
@faddat faddat requested a review from dshulyak as a code owner May 24, 2021 08:13
@faddat faddat marked this pull request as draft May 24, 2021 09:32
@faddat
Copy link
Contributor Author

faddat commented May 24, 2021

@ilgooz hey, my apologies. I have to brush up a thing here and will convert back from draft.

@faddat
Copy link
Contributor Author

faddat commented May 24, 2021

Closing this and starting again from the develop branch. Had it going but then merged uncleanly.

@faddat faddat closed this May 24, 2021
@faddat
Copy link
Contributor Author

faddat commented May 25, 2021

Continued in #1187

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.

3 participants