closed because not relevant anymore.

@Florian, this pull request reuses the previous variables, thus the diff should
be be smaller.
https://bitbucket.org/PatFer/issue761/pull-requests/1/issue761-rev2/diff

@Malte: I should have phrased it better. Nobody has a buggy behaviour with it
currently. Yes, we saw the security problems with executing arbitrary code. For
this security risk, I opened a new issue767 were people can discuss which format
to use, how to structure,...

> I reverted partially back to the *build_configs.py (but our build_configs.py
> is now loaded before the user defined onces), because this is not part of the
> actual issue, nobody seems to has issues with it

I disagree about the "nobody seems to have issues with it". I think several
people in the meeting we discussed this said that the current behaviour is
terrible, not far from a typical injection attack vulnerability. I certainly
share this opinion.

The way I understood it yesterday, we only wanted to move two lines from one
file to another. The patch in your pull request does a lot more. Could you
please reduce the diff to the essential change?

After the discussions yesterday here is a new pull request:
https://bitbucket.org/PatFer/issue761/pull-requests/1/issue761-revised-1/diff

I reverted partially back to the *build_configs.py (but our build_configs.py is
now loaded before the user defined onces), because this is not part of the
actual issue, nobody seems to has issues with it, and concerns with backwards
compatibility existed. If you want this change, feel free to open a new issue.

I implemented the changes and Florian reviewed it. Thank you. But he detected a
problem in our design.

We decided to have a 'debug' and a 'release' alias (which could be changed in
the build_config.py) and their binaries are (because of their names) stored
under 'debug' and 'release'. FD does not care for the alias configuration, and
just uses the binaries under release (or debug).

Florian detected that we (or the user) changes for some reason the parameters
behind default, CMake will not detect this and not rebuild the binary. He
thought that is not possible with CMake and our intended approach has to fail.

We can circumvent this with additional python code, but using a tool like CMake
and slowly glueing python code on it is in our opinion not a good idea.

Thanks, Patrick! Given the sorry state of my TODO list, can someone other than
me take over the review? (Florian is the only other person on the nosy list, but
we could also ask someone not yet on the nosy list.)

I uploaded a fix on my bitbucket for reviewing.

https://bitbucket.org/PatFer/issue761/pull-requests/1/issue761/diff

Both scripts use release resp. debug as default builds. The user can specify new
builds in custom_builds.py and under experiments/issue761 is a test suite.

I wouldn't bother with making the build callable from fast-downward.py for now.
The main reason I suggested it was that if everything was in one place, it would
solve the issue of needing the same information in two places. But we've agreed
on a different solution for this for now.

Regarding the custom script pattern: I think it's overkill to have a pattern in
the first place, and a single custom file is enough. Something like
"custom_builds.py" looks good to me. (If someone wants to use many separate
files, they would have the option of implementing that within custom_builds.py,
although we also discussed the option of changing from Python code to something
like a YAML file, in which case this would no longer be possible. But I wouldn't
worry about that for now; I don't think replace the Python-based config setting
with something else would be a big improvement at the moment.)

My remarks:
1<<Make build callable from fast-downward.py is a small modification. Refactor
the build script to a method and call it. If we want to unify the scripts later
this will mean just copying the methods. Furthermore, we can already say using
build.py is depreciated. Thus, people have time to adapt.

I would implemented this by let fast-downward.py have different modules (default
module the run). Currently there are two modules only. This allows full
backwards compatibility.
./fast-downward.py build …. 	calls build
./fast-downward.py run …		runs the FD system (used if no module given)


3<<I am not convinced by the custom script pattern (will be just a variable to
change). Any other suggestions?
3.1 build_config_*.py
3.2 build_aliases_*.py
3.3 custom_builds_*.py
3.4 custom_build_configs_*.py

Results from FD Meeting:

1>>Shall we unify ‘fast-downward.py’ and ‘build.py’?
The general opinion was no (with thoughts on backwards compatibility). The
suggestion was made to be able to call build from ‘fast-downward.py’.


2>>How to synchronize the difference default release/debug builds in those scripts?
In the future we will get ride of handling 32 and 64 bit versions, thus, the
suggested solution was to define like today different build aliases. Two alias
will be named ‘release’ and ‘debug’ (thus their binaries will be in the
directories ‘release’ and ‘debug’). Both scripts will use these builds as their
default builds. Thus, ‘build.py’ knows the default build parameters.
‘fast-downward.py’ does not care about the parameters and just uses the
executables in the default directories.
We get ride of the last lines in ‘build_config.py’ where we can define which
aliases to build as defaults. This implied that the whole system would use the
debugs which it did not (only the build used it). Instead the aliases ‘debug’
and ‘release’ are used as defaults.

3>>Defining custom build aliases?
Previously, the system loaded all files matching ‘*build_config.py’ and executed
them. The user could create new files matching the pattern to define new
aliases. Furthermore, it could overwrite the default build names.
It was now suggested to change the matching pattern to better differentiate
between the original build_config script and the custom scripts of the user. The
original build_config will be loaded first.
The user can still define any build alias as default by overriding the previous
default alias.
The suggested custom script pattern was ‘build_config_*.py’.

This is probably a topic where opinions on the best way to go vary, so I suggest
talking to the others to find out what a good design here could be.

We should not necessarily restrict ourselves too much by the current design. If
I recall correctly, build.py was implemented very quickly without too much
discussion of the design because we needed a new build script when we switched
to cmake. For example, if we want to share functionality regarding build
configuratoin between fast-downward.py and build.py, one option is to unify the
scripts completely. There is no reason why the build could not be triggered by
something like "./fast-downward.py --build" rather than having a separate build
script.

Now with the summary as message:

Building FD uses the default configurations in "build_configs.py".
Running FD has the defaults "release32" and "debug32" hardcoded. 

Providing everytime the build option is an annoyance.

I suggest uses the defaults specified in "build_configs.py".

(Example when the problem occurs: change default build (e.g. to release64) in
build_config.py, build and if we then run it, it will fail, because the driver
tries to use release32).


Possible solutions:
1) load module "build_configs.py" in arguments.parse_args() to obtain default
debug and release builds [that would make the driver scripts dependent on the
build_config.py in an upper directory].

2) ./fast-downward.py loads the default builds and passes them as argument to
driver.main(releaseBuild="release32", debugBuild="debug32") ->
arguments.parse_args(releaseBuild, debugBuild)

Possible solutions:
1) load module "build_configs.py" in arguments.parse_args() to obtain default
debug and release builds [that would make the driver scripts dependent on the
build_config.py in an upper directory].

2) ./fast-downward.py loads the default builds and passes them as argument to
driver.main(releaseBuild="release32", debugBuild="debug32") ->
arguments.parse_args(releaseBuild, debugBuild)