Add support for overriding default compiler flags with '.nim.cfg' and '.nims' configuration files

[SekouDiaoNlp]

Hi @Pebaz and all.

In response to issue #51 regarding the inability to override nimporter's default compiler flags, I have been investigating the issue.

Basically it should be possible to override nimporter's default list of flags NIM_CLI_ARGS with values taken from a configuration file other than switches.py.

According to the nim compiler documentation, the compiler can be configured via both .nim.cfg and .nims files.

One relevant bit of the documentation is:

Configuration files
Note: The project file name is the name of the .nim file that is passed as a command-line argument to the compiler.

1. The nim executable processes configuration files in the following directories (in this order; later files overwrite previous settings):
2. $nim/config/nim.cfg, /etc/nim/nim.cfg (UNIX) or <Nim's installation directory>\config\nim.cfg (Windows). This file can be skipped with the --skipCfg command line option.
3. If environment variable XDG_CONFIG_HOME is defined, $XDG_CONFIG_HOME/nim/nim.cfg or ~/.config/nim/nim.cfg (POSIX) or %APPDATA%/nim/nim.cfg (Windows). This file can be skipped with the --skipUserCfg command line option.
4. $parentDir/nim.cfg where $parentDir stands for any parent directory of the project file's path. These files can be skipped with the --skipParentCfg command-line option.
5. $projectDir/nim.cfg where $projectDir stands for the project file's path. This file can be skipped with the --skipProjCfg command-line option.

A project can also have a project-specific configuration file named $project.nim.cfg that resides in the same directory as $project.nim. This file can be skipped with the --skipProjCfg command-line option.

Command-line settings have priority over configuration file settings.

At the moment NimCompiler.compile_nim_extension() and NimCompiler.compile_nim_code() don't check if there is a .nim.cfg or .nims file in the target directory, and because command-line settings have priority over configuration file settings, once nimporter calls nimble or nim with it's default flags on the cli, the flags in NIM_CLI_ARGS have priority over any flags the user might have defined in a .nim.cfg or .nims. This means that flags specified outside of switches.py are actually ignored by both nimble and nim.

Proposed solution:

After a bit of testing with both 'nim_1.5' and 'nim_1.6' it appears that the compiler first checks the presence of a .nim.cfg file, and if not found then checks for the presence of a .nims file.

The solution I propose is to parallel this behaviour. Forward the default cli settings only if no suitable config file has been found, this way the default values in NIM_CLI_ARGS won't interfere with user defined flags.

Modify NimCompiler.compile_nim_extension() and NimCompiler.compile_nim_code() to:

1. Check for the presence of a .nim.cfg file: If found, don't pass NIM_CLI_ARGS to nim_args. (new behaviour).
2. Check for the presence of a .nims file: If found, don't pass NIM_CLI_ARGS to nim_args. (new behaviour)
3. Check for the presence of a switches.py file: If found call get_swiches() and pass switches['bundle'] to nim_args. (current behaviour)
4. No config file found: Pass NIM_CLI_ARGS to nim_args. (current behaviour)

Let me know if you have any suggestions or comments I should be able to start implementing the changes tomorrow or on Tuesday.

Cheers,

SekouDiaoNlp.

[Pebaz]

@SekouDiaoNlp excellent write-up! I think that between all of the available options, the switches.py should be removed since I have seen most people get confused with it and I think for good reason. Originally I had intended that to be easier to use than other methods of overriding the compiler switches but now it just seems to be in the way. Other than that the proposed solution looks fantastic! :)

[SekouDiaoNlp]

Thanks for the feedback @Pebaz !

You are right that the options are many and that the switches.py is redundant. I will remove them and implement the changes starting tomorrow.

I will also update the test suite to keep the same mount of coverage.

I 'll make a Pull Request as soon as it is ready.

Cheers.

[Pebaz]

Fantastic, thank you so much!

[SekouDiaoNlp]

Hey @Pebaz , what's up?

I started implementing the fix for the lto flag on macOS and I am about to write some tests to check if everything is ok.

I can test all my changes locally, but as I don't have a mac handy, I use the same workflow file you use to automate testing via GitHub Actions.

I wanted to check the logs of your latest builds through Github Actions but the logs have been deleted as they are too old.

Could you please re-run some of the latest jobs, so that I can check them out and compare them with mine please?
If you could re-run these ones:

• The latest master build
  • https://github.com/Pebaz/nimporter/actions/runs/600288083
• Those CI test builds to have a view of some of the failure modes
  • https://github.com/Pebaz/nimporter/actions/runs/825579961
  • https://github.com/Pebaz/nimporter/actions/runs/825559181
  • https://github.com/Pebaz/nimporter/actions/runs/825502118

That would be awesome.

Cheers.

[SekouDiaoNlp]

For example the workflow fails with main.yml as is on master with nim-version: '1.0.6': https://github.com/SekouDiaoNlp/nimporter/actions/runs/1408946643

However, the workflow is successful with nim-version: 'stable' : https://github.com/SekouDiaoNlp/nimporter/runs/4071119710?check_suite_focus=true

[Pebaz]

Oh shoot it looks like it won't let me rerun some of these jobs. It won't even let me rerun my latest one for some reason.
It's totally cool if you need to add builds to the .yml for other branches. For instance, you could add your current feature branch to the .yml to run all the builds on that branch for testing and then just take it out for later. I think that's what I had to do also since this repo basically does trunk-based-development. Alternatively, it might even be worth making a dedicated dev branch that we use for testing changes before merging 🤷

[SekouDiaoNlp]

Yes I already created a dev branch on my fork and tweaked the .yaml file to run the jobs on Python 3.9 and 3.10 as well as nim 1.4, 1.5 and devel on top of stable.

For some reason, the build on nim 1.5 systematically fail I will investigate that later.
See https://github.com/SekouDiaoNlp/nimporter/actions/runs/1409422787

Otherwise all other permutations of the matrix build successfully.

I already implemented the macOS fix as well as nim config file detection.
I am in the process of writing the tests for has_nim_config(). When I am done, I will just have to remove the code handling switches.py and refactor the duplicated code in nimporter.py for better maintainability. After updating the docs we should be good to go.

[SekouDiaoNlp]

See #55