Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow settings paths through env #226

Merged
merged 10 commits into from
Jul 11, 2021

Conversation

robamu
Copy link
Contributor

@robamu robamu commented Jul 3, 2021

Builds on #187 , documentation extended.

closes #187

@robamu robamu changed the title allow settings paths through env WIP: allow settings paths through env Jul 3, 2021
@robamu robamu changed the title WIP: allow settings paths through env allow settings paths through env Jul 3, 2021
@robamu robamu changed the title allow settings paths through env allow settings paths through env and README improvements Jul 3, 2021
@robamu robamu changed the title allow settings paths through env and README improvements allow settings paths through env Jul 4, 2021
Copy link
Collaborator

@atsju atsju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Could almost be merged as is .

README.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -37,7 +37,9 @@ It will compile a project for the `F4` family by default, but you can also compi
# Usage

First of all you need to configure toolchain and library paths using CMake variables.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could add the variables names (or an example of variable name) here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good idea. maybe even a concrete example for a file which can be sourced to load all necessary env variables at once (I find this really convenient compared to very large CMake commands, and the path stays clean if it is not used)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of commit 08ceb4b it seems already excellent to me. It's better than what I expected. I will review When you remove WIP.

Please note also there are also Windows users like me, so keep generic (even if we can consider Linux as default choice)

Copy link
Contributor Author

@robamu robamu Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually used .sh scripts on Windows with git bash or MinGW64. I am not totally sure whether this belongs in the README, but a regular CMake workflow generally involves more scripting and I found scripts like that to set up environments quickly extremely useful. I added this for the powershell on Windows as well, but I need to test this. I read somewhere that it would not be possible with powershell like that

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .sh only is OK don't worry. I just wanted to heads up. Your work is great.

@atsju atsju requested a review from Hish15 July 5, 2021 08:40
robamu and others added 2 commits July 6, 2021 03:10
Co-authored-by: Julien Staub <atsju2@yahoo.fr>
@robamu robamu changed the title allow settings paths through env WIP: allow settings paths through env Jul 6, 2021
@atsju atsju marked this pull request as draft July 6, 2021 11:51
@atsju atsju changed the title WIP: allow settings paths through env allow settings paths through env Jul 6, 2021
@atsju
Copy link
Collaborator

atsju commented Jul 6, 2021

No worry I just test some Github features :)

@robamu
Copy link
Contributor Author

robamu commented Jul 6, 2021

Okay, all okay now

@robamu robamu marked this pull request as ready for review July 6, 2021 13:43
@atsju atsju self-requested a review July 6, 2021 13:52
Copy link
Collaborator

@atsju atsju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hish15 Please review. To met this is 🥇
Plus it's super helpful. Now I can set the path to cubes and easily run tests on branches without additional script or cmakelist modification

@Hish15
Copy link
Collaborator

Hish15 commented Jul 11, 2021

Hi all,

I understand the need covered by this feature, but isn't this what cmake toolchain files are meant?
My second concern is that defining things in ENV bring the risk of using unwanted configuration (from a previous project for instance)

I am not against the feature, but needed to open the discussion to be sure we are headed where we want.

@atsju
Copy link
Collaborator

atsju commented Jul 11, 2021

Hi all,

I understand the need covered by this feature, but isn't this what cmake toolchain files are meant?
My second concern is that defining things in ENV bring the risk of using unwanted configuration (from a previous project for instance)

I am not against the feature, but needed to open the discussion to be sure we are headed where we want.

Fair point. I would argue this way:

  • Toolchain file are more for configuring toolchain here we are just giving path to code (And only if not already set by project)
  • This is an additional feature and users can still choose to configure via toolchain files or -D or cache or set in cmakelist (this is what I would recommend in production + HAL as submodule)
  • Only paths are set, no configuration. So the risk is limited.

As I see benefits with limited risks of regression and user errors I would recommend integrating it.

@Hish15 Hish15 merged commit a783571 into ObKo:master Jul 11, 2021
@robamu robamu deleted the garjones/allow_settings_paths_through_env branch July 12, 2021 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants