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

Move away from committing an autogenerated script #13

Closed
rgilton opened this issue Sep 27, 2020 · 7 comments
Closed

Move away from committing an autogenerated script #13

rgilton opened this issue Sep 27, 2020 · 7 comments

Comments

@rgilton
Copy link

rgilton commented Sep 27, 2020

Hi, this tool is really useful! Would it be possible for you to commit the tooling that generated the build script?

@bepaald
Copy link
Owner

bepaald commented Oct 6, 2020

Hi! Thanks for your report.

Would it be possible for you to commit the tooling that generated the build script?

The program that generates the build script is quite a big project (its main function is not to generate build scripts, it is to actually compile code (multithreaded, tracking dependencies and file changes, scanning for necessary libraries to link to etc...)). It is definitely not meant for the general public. If I were ever to release it, it would not be as a part of signalbackup-tools, but as a separate project. But that is for a distant future: I have been using this tool for over ten years and I have a long list of changes planned for the program to make it better, faster more robust and make the generated scripts a lot nicer to read and faster as well, but unfortunately my time is limited and when I do have a moment, other issues and projects are more important right now.

Move away from committing an autogenerated script

Is there any particular reason to not want the autogenerated script in the project? I fully admit it's not very readable, but it's just a harmless little extra for anyone not willing to write the compilation command by hand, it's not actually necessary to use it. Would it help if I typed out the script manually whenever the source changes?

@rgilton
Copy link
Author

rgilton commented Oct 11, 2020

Hi @bepaald. The main reason it'd be good to have the tooling that generated the build script committed is that it becomes possible for others to easily modify the build process, and submit patches to you to improve the build. At the moment, if I were to need to changes the flags passed to the compiler, then I have to edit tens (hundreds?) of lines that repeat those flags, and the resulting patch would be useless to you, as if you accepted it then you would no longer be able to use the aforementioned tooling. Since such patches couldn't be integrated by you, other people have to keep rebasing their set of build patches on top of any changes you make and resolving whatever conflicts are involved.

This is all based on the assumption that you would like others to use and work on this signal backup tool -- if this isn't the case then I apologise for making that assumption.

@bepaald
Copy link
Owner

bepaald commented Nov 1, 2020

buildscript_new.txt
Well, if you are hacking on the program (and please feel free to), then

  1. You needn't use the buildscript. It was really meant for people who don't know what a 'compiler' is or are scared of typing long commands in the terminal. It provides zero benefit to you, it is not faster or better in any way. Just type g++ */*.cc *.cc -lsqlite3 -lcrypto and add any flags you feel necessary. I think the only needed flag is '-std=c++20' (but 17 will also work if your using an older compiler).

  2. The program should really compile with any flags (or no flags at all, except a recent std). The ones in the script are just the ones I happen to use by default myself. If the program has problems when compiling with some (normal) flag, or needs one to compile properly, I think I would consider that a bug in the program.

  3. If there was ever any need to change the flags, I indeed can't apply any submitted patches, but it is still trivial for me to adjust the flags in the script, so simply opening an issue to inform me would work just fine.

Having said all that:
I do understand you point. I have been busy working on a new (autogenerated) buildscript that hopefully addresses most of your issues. Maybe you would like to try it out? In this script the flags (and sources) are in separate variables at the top of the script for easy editing. The way the variables are written even allows to change them without editing the script, for example: $ CXX="g++-9" CXXSTD="-std=c++17" CXXARCH=" " bash buildscript_new.bash. Also, on supported shells (bash > 4,4), it could be much faster on multiprocessor systems by compiling in a parallel for-loop. It is a work-in-progress, but I'd love to hear what you think and if it takes away some of your concerns.

buildscript_new.txt Renamed with .txt extension because github won't have any other way)

@taggart
Copy link

taggart commented Jan 12, 2021

Also the buildscript has some bashisms in it that prevent it from running with dash (which is the default /bin/sh on debian and some other distros). Here's the output of the check-bashisms tool:

$ checkbashisms BUILDSCRIPT.sh 
possible bashism in BUILDSCRIPT.sh line 10 (alternative test command ([[ foo ]] should be [ foo ])):
while [[ $# -gt 0 ]] ; do
possible bashism in BUILDSCRIPT.sh line 11 (alternative test command ([[ foo ]] should be [ foo ])):
    if [[ "$1" == "--config" ]] && [[ $# -gt 1 ]] ; then
possible bashism in BUILDSCRIPT.sh line 11 (should be 'b = a'):
    if [[ "$1" == "--config" ]] && [[ $# -gt 1 ]] ; then
possible bashism in BUILDSCRIPT.sh line 16 (should be VAR="${VAR}foo"):
            EXTRAOPTIONS+="$1"
possible bashism in BUILDSCRIPT.sh line 18 (should be VAR="${VAR}foo"):
            EXTRAOPTIONS+=" $1"
possible bashism in BUILDSCRIPT.sh line 24 (should be 'b = a'):
if [ "$CONFIG" == "clang" ] ; then
possible bashism in BUILDSCRIPT.sh line 378 (should be 'b = a'):
if [ "$CONFIG" == "cryptopp" ] ; then
possible bashism in BUILDSCRIPT.sh line 732 (should be 'b = a'):
if [ "$CONFIG" == "default" ] ; then
possible bashism in BUILDSCRIPT.sh line 1086 (should be 'b = a'):
if [ "$CONFIG" == "windows" ] ; then

But better would be to replace this script with a Makefile or something

@taggart
Copy link

taggart commented Jan 12, 2021

Also it would be nice to be able to run "make clean" to be sure all the old .o's etc are gone, have common flags at the top where you could change them in one place, not have to rebuild things that are already built with no dependency changes, etc.

@taggart
Copy link

taggart commented Jan 12, 2021

Also, the default compiler section could use an $EXTRAOPTIONS in the final linking line so that it's possible to just set EXTRAOPTIONS="-lstdc++fs" at the top when using older g++

@bepaald
Copy link
Owner

bepaald commented Jan 13, 2021

Also the buildscript has some bashisms in it that prevent it from running with dash (which is the default /bin/sh on debian and some other distros). Here's the output of the check-bashisms tool:

Ok, I've fixed those for now. The script should work with dash now, thanks!

But better would be to replace this script with a Makefile or something
Also it would be nice to be able to run "make clean" to be sure all the old .o's etc are gone, have common flags at the top where you could change them in one place, not have to rebuild things that are already built with no dependency changes, etc.

Agreed, but I have neither the time, nor the skill to write and maintain a Makefile. I do have a newer version of a buildscript that has some advantages. It looks a lot cleaner and more comprehensible, it has the flags at the top for easy editing (though the flags can also be set at the command line without editing the file itself), it uses all available processor cores and it only recompiles files when the object is out of date. One disadvantage though: it needs bash (or compatible, certainly not dash). Maybe you would like to test it and give an opinion?
buildscript_new.txt

Also, the default compiler section could use an $EXTRAOPTIONS in the final linking line so that it's possible to just set EXTRAOPTIONS="-lstdc++fs" at the top when using older g++

Yes, that is indeed a good idea, I might get to work on that soon. But, in the meantime - as I've said before - if you want to fiddle with the compiler flags, the buildscript is basically not for you (the current one at least, maybe the new one will be). It was originally just provided because I realized a relatively large group of Windows users might want to try the program, who have no idea what 'compiling' is or how to do it (at that time, Windows builds were not possible, so compiling and running under Linux was the only option). If you are capable of typing g++ */*.cc *.cc -lcrypto -lsqlite3 (-lstdc++fs), the script provides no benefit at all.

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

No branches or pull requests

3 participants