-
Notifications
You must be signed in to change notification settings - Fork 64
Some Quality of Life changes for external_deps/build.sh
#1789
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
Conversation
b02dfcb
to
096d1b6
Compare
external_deps/build.sh
Outdated
\tbuild-linux — platforms buildable on linux: ${linux_build_platforms} | ||
\tbuild-macos — platforms buildable on macos: ${macos_build_platforms} | ||
\tall — all platforms | ||
\tevery-linux — Linux platforms: ${linux_platforms} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not all-foo
? That would be consistent with all
, and every-foo
sounds kind of ungrammatical.
Though it doesn't make much sense to have the all
platform in the first place since there is no host platform that can build all platforms, except perhaps for package
if you copied the files from somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
all-foo
? That would be consistent withall
, andevery-foo
sounds kind of ungrammatical.
I can do that.
Though it doesn't make much sense to have the
all
platform in the first place since there is no host platform that can build all platforms, except perhaps forpackage
if you copied the files from somewhere else.
Yes I use ./build.sh all package
as all my systems share the same file system (over the network when a VM or a different machine, or using direct access in case of chroot), and it's faster to run such packaging task on the file server. This is why all
exists in the first place.
For information, the other every-*
systems (we can rename all-*
) are in fact decided by the different environment we build the release binaries, so basically the different docker images, or ones built the same way. Debian Bullseye with GCC for Linux binaries, Debian Trixie with MinGW for Windows binaries, Ubuntu Jammy with AppleClang in Darling for macOS (I actually use a real macOS for the deps, but that's the exception).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually linux
, windows
, macos
do the job very well too. I did that instead.
external_deps/build.sh
Outdated
'genlib'|'depcheck') | ||
log STATUS "Running ${pkg} for ${PLATFORM}" | ||
;; | ||
*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other non-building targets for example wipe
. Anyway this is needless complexity. Just print Processing target '${pkg}' for ${PLATFORM}
and you get 99% of the benefit with 1 line of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea! Now done.
mkdir -p "${2}" | ||
case "${1}" in | ||
local archive_file="${1}"; shift | ||
local extract_dir="${1}"; shift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of calling shift
here? x="${1}"; y="${2}";
seems easier to follow, not requiring the reader to sequentially process the shift
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to process the shift more than you don't have to process the stack when reading this in C:
int extract(char* archive_file, char* extract_dir)
{
Or even in older C:
int extract(archive_file, extract_dir)
char* archive_file;
char* extract_dir;
{
Actually using shift
here means the variables are named in the order of appearance and that you don't have to count at all, like in most languages as described above, so it actually brings less context switching for the reader.
I also used that shift
development pattern in all the rest of the code, so this is unifying the code and also reduces context switching for the reader within that source file itself.
1f8f123
to
66a011f
Compare
Print to stderr. Reformat the help message. Replace dynamic tabs with spaces. Also simplify an overengineered madness. Also we don't do zip anymore, tarball for everyone for a long time.
6d9ee6a
to
b878f1e
Compare
13ce679
to
6c6b12e
Compare
I added a commit rewording the error and help messages, it actually fixes a bug too. |
I also reworded and reformatted the help message a bit. |
6c6b12e
to
b52345c
Compare
LGTM |
Some Quality of Life changes for
external_deps/build.sh
Extracted from #1433 before merging:
The only new commit that was never made public before is
external_deps: improve extract logging
.Edit: It's a revolution!