Qucs / qucs

Qucs Project official mirror
http://qucs.sourceforge.net/
GNU General Public License v2.0
1.16k stars 213 forks source link

Fix optimization #800

Closed in3otd closed 6 years ago

in3otd commented 6 years ago

Optimization was no longer working properly after #730, since ASCO expects to find qucsator in the path. Besides, optimization never honored the QUCSATOR environment variable and always called just qucsator. To fix this, the ASCO "general" optimizer mode is now used, where it actually runs a general.sh script, which needs to do the actual simulator invocation. The script is created by Qucs with the correct simulator path and name before running ASCO.

All this works fine in Linux but not on Windows, since there ASCO does not use the right path separator and script extension. I've contacted the ASCO maintainer about this. If needed, I'll add a patch for ASCO to fix the Windows behaviour in this PR later.

felix-salfelder commented 6 years ago

could you please move the ifdefs to platform.h?

#ifdef __WIN32__
#define QUCS_BATCH_FILE_EXTENSION .bat
#else
#define QUCS_BATCH_FILE_EXTENSION .sh
#endif 

(etc.)

in3otd commented 6 years ago

um, I guess the Right Thing would be to clean up all the #ifdef __MINGW32__ and this will be quite some work.

Currently there are other files which end in .bat in Windows but have no extension for Linux, should these be changed too?

felix-salfelder commented 6 years ago

i agree about the Right Thing. however, as with indents, it is more practical to only get it right during edits, in those places that are edited.

it will be nice to use QUCS_BATCHFILE_EXT globally. if it is blank in many non mingw cases already, then it will be more natural to go with a blank here, too.

i assume (hope) that asco does not specifically require ".sh" here. if it does, that would be a new variable (also in platform.h, QUCS_BATCHFILE_EXT_ASCO_EXCEPTION).

in3otd commented 6 years ago

i assume (hope) that asco does not specifically require ".sh" here.

it does...

felix-salfelder commented 6 years ago

eek.

okay, forget about platform.h.

just #define whatever you need (depending on MINGW) but at the top of the file you are changing, just not scattered all over the place.

bonus points if you use static strings instead of preprocessor constants.

#ifdef MINGW
static std::string asco_script_filename="general.bat";
/// more here
#else 
static ... .sh
/// more here
#endif
in3otd commented 6 years ago

I'm a bit lost on what's the Best Thing to do here; the script names are OS-dependent and also its content, written by Qucs, is. For the moment, I've just regrouped things a bit and moved the script name definition at the top of the file but I do not see why it's not better for that to be local to the function where it's used.

felix-salfelder commented 6 years ago

i agree. lets rebase, CI and merge.

in3otd commented 6 years ago

rebased

felix-salfelder commented 6 years ago

anything else? (there's still "WIP" in the headline..)

in3otd commented 6 years ago

ops, I need to remove that :grin: but this reminded me that I need to add a patch for ASCO, since on Windows it uses the wrong path separator for calling the script so its execution fails. I'll put it in contrib/windows and apply it when building the Windows package in #801

in3otd commented 6 years ago

done - I've added the patch to #801.