Laszlo Gaal has posted comments on this change. ( http://gerrit.cloudera.org:8080/20519 )
Change subject: IMPALA-12354: Add aarch64 native-toolchain build ...................................................................... Patch Set 5: (4 comments) Thanks for the extension! As far as the toolchain storage structure is concerned I would prefer to see a more explicit distinction than just a different TOOLCHAIN_BUILD_ID value between binary packages targeted at different CPU architectures. This patch can get us started though. I also added requests for comments at a few locations. http://gerrit.cloudera.org:8080/#/c/20519/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20519/5//COMMIT_MSG@10 PS5, Line 10: Could you add some high-level details on the structure you're building here? Most Impala devs don't touch the toolchain often, so a summary could help their understanding on how this changes the toolchain layout and the way Impala picks the right bits to download. http://gerrit.cloudera.org:8080/#/c/20519/5/bin/bootstrap_toolchain.py File bin/bootstrap_toolchain.py: http://gerrit.cloudera.org:8080/#/c/20519/5/bin/bootstrap_toolchain.py@257 PS5, Line 257: arch = platform.machine().upper() Would it make sense to add a check for the known and supported architectures and an explicit error message if something unknown is encountered? This would be easier to understand than having to debug a failed download. http://gerrit.cloudera.org:8080/#/c/20519/5/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/20519/5/bin/impala-config.sh@88 PS5, Line 88: export IMPALA_TOOLCHAIN_BUILD_ID_X86_64=25-ba9ab5f36c : export IMPALA_TOOLCHAIN_BUILD_ID_AARCH64=4-ba9ab5f36c What happens if the build numbers happened to clash? Would it be reasonable to add the architecture tag to the directory structure/file names for non-x86 packages? Currently the toolchain's naming convention requires two correct "coordinates" for a package download: you have the toolchain build ID and the package version, and both are part of the package's complete URL. If you accidentally misconfigure one of them, you end up with a download error instead of the wrong version of the package. Ideally I'd like to see something similar for the architecture as well, since that is just another dimension for which you need to provide a coordinate: x86 or ARM? (or maybe something else, who knows). Adding the arch to the package name somewhere could safeguard against mismatched libraries a lot earlier than linker errors. I do realize that this would require a change in native-toolchain, so all this can be handled in follow-up patches, but it would be great to add some mechanism like this. http://gerrit.cloudera.org:8080/#/c/20519/5/bin/impala-config.sh@95 PS5, Line 95: #*- Could you add a brief comment here pointing out the Bash magic? It's subtle, some folks may miss it -- To view, visit http://gerrit.cloudera.org:8080/20519 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9bfa7125dbc647b33041c5572d97b7f7ccad6258 Gerrit-Change-Number: 20519 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Thu, 05 Oct 2023 20:07:50 +0000 Gerrit-HasComments: Yes
