FMCorz / mdk

Moodle Development Kit. A collection of tools meant to make developers' lives easier.
GNU General Public License v3.0
85 stars 47 forks source link

Set-up ghostscript automatically when initialising behat and adding better way to find executables #141

Open andrewnicols opened 8 years ago

andrewnicols commented 8 years ago

After I wrote my first version of this I took a step back and decided to tackle one of the more irritating issues I've been frustrated by - the fact that the defaults in config-dist.json are so specific for paths.

I've adjusted it to use the find_executable function of distutils.spawn, which checks up the $PATH as your shell does.

Of course, if you have a non-null, non-empty value in the config.json (or config-dist.json) it will prefer that, but otherwise it will do something sane. This means that my gs install in /usr/local/bin doesn't require specific config.

I've haven't yet expanded this to all executables, I thought the dust should be allowed to settle. I also think that mdk doctor could go through all known executables in the config and compare them against the version from find_executable and remove them from the config if they match as a sort-of upgrade step, but this is obviously dependent on all of the other variables being used.

FMCorz commented 8 years ago

Hi Andrew,

Why is that sitting in the pull request "Add an alias for mdk theme to set the instance theme"?

FMCorz commented 8 years ago

Looking at the patch, I am not sure I agree that Behat init should automatically set the path to gs. mdk behat is supposed to initialise Behat, not to change other settings or add additional ones. What if you want to set the path to ghostscript to something else, you wouldn't be able to use mdk behat any more. How do users with a custom gspath set via the admin settings test behat? Do they have to override their prod value for testing?

Maybe what we need is a $CFG->behat_init_cfg = [ ... ] instead, in which case that should be added to MDK. Ping @rajeshtaneja.

danpoltawski commented 8 years ago

Maybe what we need is a $CFG->behat_init_cfg = [ ... ] instead, in which case that should be added to MDK. Ping @rajeshtaneja.

Not properly looked at what you are asking but $CFG->pathogs is whitelisted and passed through from the main config.php setting. See https://github.com/moodle/moodle/blob/master/lib/behat/lib.php#L164

rajeshtaneja commented 8 years ago

Dan is correct, we moved system executable paths out-of-plugin and white listed them in behat_clean_init_config() in MDL-44839

So setting $CFG->pathogs to proper executable is enough. Not sure if finding the executable and using it is a good option or asking user about it as this is only expected during mdk init process.

andrewnicols commented 8 years ago

Hi all,

Not sure what happened with the title of this pull request. I filed it using GitHub's own hub tool.

As both Dan and Rajesh say, pathtogs is one of the whitelisted configuration settings, however perhaps mdk behat is the wrong place for this to be set.

Perhaps we need a way of setting these in Moodle from a new mdk command. Unless I'm missing something, it isn't possible to use an mdk script here because it won't have access to the mdk config.json in order to set the values. In any case, I feel that we need an easy way of setting all stock values in your Moodle configuration for these kinds of paths (and so perhaps it can be called as an addendum to mdk install).

I feel that we should try and use the system defaults where possible - we currently do not provide either an initial setup question asking for these values, nor an upgrade script to ask for them. At the moment that isn't an option. The stock value of pathtogs in Moodle core is '/usr/bin/gs'. The value from homebrew is '/usr/local/bin/gs'. As a result, any dev running tests on a mac will have to remember to set the value every time they install a new instance.

The default values in config-dist.json are very specific to Fred's dev environment (I presume). Some of them are different to the locations you'd get with brew, and probably different in some areas to Fedora. I believe that we should use the versions that you would use as a developer in your shell environment. When I call grunt through mdk (e.g. mdk css -c), I expect that to use the same version of grunt as I would get if I called grunt manually.

The changes I've made here use the values in your config.json, fall back on the config-dist.json, and only then if the value was not found do they go to the shell. For a majority of users, they will not cause issues. There are just a handful of cases where you'd have to fettle things manually and I can't think of any specific examples.

The reason that I went for putting this in mdk behat was because ideally you want all behat tests to run all the time. You don't want to miss a substantial chunk the mod_assign tests because your pathtogs is missing.