[WIP] Add a settings file to configure cobc at runtime #117
Conversation
New --settings=FILE can be used to configure flags that are normally set at compile time. These settings can be used typically to create cross-compilers by retargetting an existing compiler. Also add a setting to make the compiler relocatable, i.e. compute paths of include, share, etc. relative to cobc's location.
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## gcos4gnucobol-3.x #117 +/- ##
=====================================================
- Coverage 65.74% 65.49% -0.25%
=====================================================
Files 32 34 +2
Lines 59092 59345 +253
Branches 15575 15679 +104
=====================================================
+ Hits 38849 38869 +20
- Misses 14262 14457 +195
- Partials 5981 6019 +38
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| #ifdef COB_DEBUG_FLAGS | ||
| COBC_ADD_STR (cobc_cflags, " ", cobc_debug_flags, NULL); | ||
| #endif | ||
| if (!cb_setting_COB_DEBUG_FLAGS[0]){ |
There was a problem hiding this comment.
Access to this variable before it might have been modified by loading the settings.
There was a problem hiding this comment.
sounds like an egg-and-hen problem
There was a problem hiding this comment.
This is about https://sourceforge.net/p/gnucobol/feature-requests/450/ where additional important discussion happens. Please outline more about the need for this configuration there, keeping in mind that if you want to cross-compile for another environment you still have to have:
- GnuCOBOL built for that cross-target (to get "libcob"; normally this will also provide you with a "cobc" for that environment)
- a C cross-compiler (that you've likely used to generate that GnuCOBOL version in the first place, so all of its configuration is already inbuilt in the generated cobc executable)
The main culprit: when building GnuCOBOL itself with a cross-compiler you need a subsystem to also execute the resulting cobc binary (for example wine).
I do wonder if it isn't more reasonable to work on https://sourceforge.net/p/gnucobol/feature-requests/245/ which allows to not only cross-compile GnuCOBOL but also provide the option to generate a cross-compiling cobc.
A "side issue" here is the option to be relocatable, but that does only apply here to the compiler, not to the runtime.
I suggest to keep that out of this PR and instead improve the relocatable option that Ron already added in trunk (the "get name" function for example would be a reasonable addition).
Note: I do think that making GnuCOBOL well relocatable is important and this may also land in 3.x (likely to only happen if you provide a patch), but as we have that in trunk already it seems more reasonable to test and improve there (it was only tested with GNU/Linux x64), then either merge the revisions to 3.x or do a big backport instead.
If we still want to add a compiler setting, wouldn't it then be reasonable to use the existing configuration options in libcob?
To do so, the current entry functions in config.c would get an additional pointer to the configuration structure (we even can store it in a static var to ease access later on) and and we'd have a separate configuration table that only contains the cobc related settings, then pass the that to the config functions in libcob to set the values (single place of already well tested code, with guaranteed identical behavior in the future).
Nonetheless - first review while still in draft
I highly suggest to add ChangeLog entries at least before each push, this way you know more what you've changed and why, and it also helps for a possible review.
| @@ -592,6 +583,7 @@ static const struct option long_options[] = { | |||
| {"O2", CB_NO_ARG, NULL, '2'}, | |||
| {"O3", CB_NO_ARG, NULL, '3'}, | |||
| {"Os", CB_NO_ARG, NULL, 's'}, | |||
| {"settings", CB_OP_ARG, NULL, CB_FLAG_GETOPT_SETTINGS}, | |||
There was a problem hiding this comment.
That should be a required argument, and checking it below then take it for granted.
cobc --info should print the active setting, so cobc --settings=something --info would automatically print it.
See cobcrun --runtime-config and cobcrun --config=file --runtime-env.
There was a problem hiding this comment.
OK, after inspecting settings.c, this actually makes sense. From my gut it feels like a separate --export-settings[=<file>] would be more appropriate with defaulting to - (stdout).
In any case - if cobc --settings is added then the same feature should work with cobcrun --runtime-config (either without a parameter or as separate export option) providing a way to "dump" the settings, allowing the to be adjusted and then used.
We may want to export/dump the settings (in both the cobc and cobcrun case) match the "installed defaults" as a comment line (just uncomment, then adjust)
| #ifdef COB_DEBUG_FLAGS | ||
| COBC_ADD_STR (cobc_cflags, " ", cobc_debug_flags, NULL); | ||
| #endif | ||
| if (!cb_setting_COB_DEBUG_FLAGS[0]){ |
There was a problem hiding this comment.
Shouldn't cobc_debug_flags be part of cb_setting_COB_DEBUG_FLAGS?
cobc/settings.def
Outdated
There was a problem hiding this comment.
The file needs a header and the strings should be gettextized.
Items not used should be removed (or commented out, with their strings not gettextized).
| #elif defined (HAVE_MPIR_H) | ||
| #define MATH_INCLUDE "#include <mpir.h>" | ||
| #else | ||
| #error either HAVE_GMP_H or HAVE_MPIR_H needs to be defined |
There was a problem hiding this comment.
that compile-time of cobc check would then go to settings.c, the matching runtime check should also be done only once, likely there, not for each program outside of the setting code
|
|
||
| * Use of the argument `--settings=FILE` | ||
| * Use of the environment variable `COBC_SETTINGS` | ||
| * Use of `../etc/gnucobol/gnucobol.settings` from the directory |
There was a problem hiding this comment.
Shouldn't this be the COB_CONFIG_DIR (with its fallback) instead?
There was a problem hiding this comment.
I'm not sure that we need a setting for that, why not always have that?
|
|
||
| If a file is specified, an error while loading the file is fatal. | ||
|
|
||
| Once the settings have been loaded, a few variables can be modified if |
There was a problem hiding this comment.
I suggest to make that not depending on COBC_IS_RELOCATABLE (env var or its default) but instead just checking if the executable is in the place that we expect - if it isn't then o the relocation.
... but this part should possibly go elsewhere (see main review comment)
New --settings=FILE can be used to configure flags that are normally set at compile time. These settings can be used typically to create cross-compilers by retargetting an existing compiler.
Also add a setting to make the compiler relocatable, i.e. compute paths of include, share, etc. relative to cobc's location.