Suggestions for changes to build script

[malob]

I know that you aren't currently accepting pull requests, but I figured I'd use a pull request rather than an issue to suggest changes to the build script, since it's easier to talk about the changes using the diff.

I'll make review comments on the diff with explanation/motivation for each changes.

[bepaald]

I know that you aren't currently accepting pull requests, but I figured I'd use a pull request rather than an issue to suggest changes to the build script, since it's easier to talk about the changes using the diff.

I'll make review comments on the diff with explanation/motivation for each changes.

Yeah that's fine.

First: a little known fact is I don't actually manually write the build scripts myself. I have a build tool (that I did write myself) which I always use to compile all my code. At some point - I think it was specifically for this project, since I made it public - I gave the tool the functionality to generate a script to do the compile.

The tool generally compiles everything I write without any input from me, but for certain special cases, it reads a file which adds options and configurations. For this program, there are (or were) configurations for a windows build, a build using cryptopp instead of openssl and one for clang. But, because the old script version grew to insane proportions with all these build configs, and I figured nobody used the script to build the windows and cryptopp versions and recent clang versions don't need any special config anymore (supporting all default flags that gcc does), I removed the extra configs from the scripts.

This explains the presence of the CONFIG="default"-part.

Now, I've re-enabled the configs (only for the new script version, since that does not grow in size as much for each extra configuration). I've also created config options for darwin and nixpkgs based on your comments above. I'll attach the version as it is generated now, so you can test it and give feedback, I may not have got all options correctly.

num_jobs is used in both branches of the if statement below, so it should be defined above. At the moment, the build fails when using Bash with a version below 4.4 because of this.

Thanks! That's obviously a bug. Fixed now.

This is just a nit. I couldn't figure out why [ ! -z ${BASH+x} ] was required, so I just removed it, and using { .. ;} rather than (..) is a little more efficient since the former doesn't use a sub shell.

Ok, I changed the (..), I did not know about that. We can't simply check BASH_VERSINFO without first checking if we are running bash at all. For example on dash:

$ if ( { [ "${BASH_VERSINFO[0]}" -ge 4 ] && [ "${BASH_VERSINFO[1]}" -ge 4 ] ; } || [ "${BASH_VERSINFO[0]}" -gt 4 ]) ; then echo "YES" ; fi
dash: 8: Bad substitution
$ #but
$ if [ ! -z ${BASH+x} ] && ( { [ "${BASH_VERSINFO[0]}" -ge 4 ] && [ "${BASH_VERSINFO[1]}" -ge 4 ] ; } || [ "${BASH_VERSINFO[0]}" -gt 4 ]) ; then echo "YES" ; fi
$ # good!

Now, as to why I'm checking for $BASH like that exaclty? ... I don't remember, haha. But it was for a good reason after hours of stackoverflow questions, so I think I'm leaving it in.

Another nit, the $ isn't required here.

Fixed.

Thanks for your input. Let me know if the attached script works for you (run it with --config nixpkgs), or if it needs more changes. Then I'll push it, and it'll be auto-generated properly in the future.

BUILDSCRIPT_MULTIPROC_NEW.txt

[bepaald]

So, I've just pushed the new new script together with some other changes, so I'm gonna close this. But please let me know if you have any more suggestions or comments. Thanks!

[malob]

Thanks for making these changes, and sorry I didn't get back to you sooner!

The one suggestion I'd make is to change "$CONFIG" = "nixpkgs" to "$CONFIG" = "nixpkgs-darwin" since the package builds without issue using the default flags on non-Darwin systems in nixpkgs.

[bepaald]

Done! Thanks again!