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

Removing qt4 capability from faust2{faustvst,jaqt,lv2} #349

Open
wants to merge 14 commits into
base: master-dev
Choose a base branch
from

Conversation

dvzrv
Copy link
Contributor

@dvzrv dvzrv commented Sep 28, 2019

This removes qt4 functionality from faust2{faustvst,jaqt,lv2} as discussed in #300.
Additionally this fixes many (but not all!) general problems (e.g. unquoted variables, etc.), found using shellcheck.

There's still plenty of tab poisoning going on in these files, too, but that can be done in a separate pull request.

It would be awesome, if these scripts could be integration tested during each pull request.

@sletz
Copy link
Member

sletz commented Sep 28, 2019

Thanks.

  • faust2jaqt does not work anymore here on OS X. Does it works for you?
  • why is "Quoting all variables" needed ?

@sletz
Copy link
Member

sletz commented Sep 28, 2019

faust2jaqt good.dsp
ERROR : cannot open file '' : No such file or directory

@dvzrv
Copy link
Contributor Author

dvzrv commented Sep 28, 2019

Well, a lot of variables are not quoted and yet they might contain e.g. paths with whitespaces in them.
This is very dangerous, when e.g. removing files recursively, as this can lead to a lot of files being removed, that shouldn't be removed.
The easiest way to circumvent this is to quote variables. This also has the upside of being lintable using shellcheck :-)

@dvzrv
Copy link
Contributor Author

dvzrv commented Sep 28, 2019

Do you have a good example to build using faust2jaqt? I have no macOS system available, so I can't really test that.
It's fairly possible, that I broke something.

$QMAKE -project "QT += widgets printsupport network" "CONFIG+=warn_off" "$CLANGOPT" "INCLUDEPATH+=$CUR" "INCLUDEPATH+=$FAUSTINC /opt/local/include" "QMAKE_CXXFLAGS=$CXXFLAGS -Wno-unused-parameter $FAUSTTOOLSFLAGS" "LIBS+=$ARCHLIB $SOUNDFILELIBS $SAMPLERATELIBS $OSCLIBS $HTTPLIBS" "HEADERS+=$FAUSTINC/faust/gui/QTUI.h" "RESOURCES+= $FAUSTINC/faust/gui/Styles/Grey.qrc" "$OSCDEFS" "$HTTPDEFS" "$QRDEFS" "$POLYDEFS" "$MIDIDEFS" "$SOUNDFILEDEFS" "$SAMPLERATEDEFS"
$QMAKE $SPEC QMAKE_CFLAGS_ISYSTEM=-I
"$QMAKE" -project "QT += widgets printsupport network" "CONFIG+=warn_off" "$CLANGOPT" "INCLUDEPATH+=$CUR" "INCLUDEPATH+=$FAUSTINC /opt/local/include" "QMAKE_CXXFLAGS=$CXXFLAGS -Wno-unused-parameter $FAUSTTOOLSFLAGS" "LIBS+=$ARCHLIB $SOUNDFILELIBS $SAMPLERATELIBS $OSCLIBS $HTTPLIBS" "HEADERS+=$FAUSTINC/faust/gui/QTUI.h" "RESOURCES+= $FAUSTINC/faust/gui/Styles/Grey.qrc" "$OSCDEFS" "$HTTPDEFS" "$QRDEFS" "$POLYDEFS" "$MIDIDEFS" "$SOUNDFILEDEFS" "$SAMPLERATEDEFS"
"$QMAKE" "$SPEC" QMAKE_CFLAGS_ISYSTEM=-I
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems, that "$SPEC" is empty on all OS, but Darwin, which means, that it can not be quoted here easily.
Relying on variables in such a way is not robust and should be changed.

faust -i -cn effect -a minimal-effect.cpp "$SRCDIR/$EFFECT" -o "$TMP/effect.h" || exit
fi
else
faust -i -json -a $ARCHFILE $OPTIONS "$SRCDIR/$f" -o "$TMP/${f%.dsp}_tmp.cpp" || exit
faust -i -json -a "$ARCHFILE" "$OPTIONS" "$SRCDIR/$f" -o "$TMP/${f%.dsp}_tmp.cpp" || exit
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as with $SPEC, $OPTIONS might be empty and therefore can not be quoted easily.
This is another case of unrobust variable use and should be changed.

@dvzrv
Copy link
Contributor Author

dvzrv commented Sep 28, 2019

@sletz Please try again with faust2ajqt. this works for me now!

@dvzrv
Copy link
Contributor Author

dvzrv commented Sep 28, 2019

There were similar issues with how CXXFLAGS and CPPFLAGS were set in faust2lv2.

@josmithiii
Copy link
Collaborator

josmithiii commented Sep 28, 2019 via email

@dvzrv
Copy link
Contributor Author

dvzrv commented Sep 28, 2019

@josmithiii thanks for the feedback.
The warning message (should) be unrelated to my pull request though.

@sletz do you have an idea what FAUSTTOOLSFLAGS is used for and where it is defined? It's being used in the scripts, but it's not defined anywhere.

@josmithiii
Copy link
Collaborator

josmithiii commented Sep 28, 2019 via email

@dvzrv
Copy link
Contributor Author

dvzrv commented Sep 28, 2019

@josmithiii judging from the locations, it's supposed to carry flags. I just don't know why there's no mention of these anywhere else.
Probably a legacy artifact :)

…qt4 functionality and searching for qmake-qt5/qmake using the shell builtin 'command'.
… when converting from dsp to cpp, if they are actually defined.

Making sure, that SPEC is only used with qmake/qmake-qt5, if it is non-empty.
…s to be able to quote them properly.

Only optionally using OPTIONS or FAUSTTOOLSFLAGS when compiling, if they are non-empty.
…o proper arrays, to which additions can just be added with the += operator. Using the arrays as separate elements with the [@] operator in e.g. faust compiler calls. Expanding to single string with [*] operator in qmake call.
…emoving quoted versions of CPPFLAGS. Adding to CPPFLAGS with += operator. Using CPPFLAGS, and OPTIONS by expanding them as several strings with the [@] operator (e.g. in faust or CXX calls). Expanding CPPFLAGS with [*] to single string in calls to QMAKE (because they are being assigned to a variable).
…XXFLAGS and CPPFLAGS into arrays, so they can be more easily handled and safely added to. Only using OPTIONS, if they are actually defined. Only using FAUSTTOOLSFLAGS, if they are actually defined. Dropping qt5 related extra check for x11extras and adding it directly to qmake call.
…strict handling of undefined variables and return code handling. Guarding all potentially undefined variables.
…rding all potentially undefined variables and defining OSCLIBS (formerly wrongly defined as OSCLIB), OSCDEFS, HTTPLIBS, HTTPDEFS, QRDEFS, POLYDEFS and MIDIDEFS.

These definitions should go into a separate LIBS/DEFS array and plainly be added to on demand.
…e stricter handling of return codes and potentially undefined variables. Guarding all potentially undefined variables. Removing debug echo for dspname.
@dvzrv
Copy link
Contributor Author

dvzrv commented Dec 13, 2019

@sletz I've rebased against current master-dev. Please check and feel free to merge at any time.

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