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

Add configurable compiler to global section #4

Merged
merged 9 commits into from
Jul 9, 2018

Conversation

jyapayne
Copy link
Contributor

Add a section in the global config to be able to specify the compiler. This makes it easy to switch to other compilers that still accept gcc's arguments, like aarch64-linux-gcc and others.

@genotrance
Copy link
Owner

This needs to be done differently - the same wrapper posted on github with its cfg file needs to be processed by different compilers, based on which user installs the wrapper. It either needs to be auto-discovered or passed to nimble/nimgen without hard-coding in the cfg.

I'm thinking something like an environment variable makes sense but it is kludgy. A nimgen global cfg file might also be an idea since everything would run through the same compiler in theory. Perhaps, nimgen should use the same compiler as specified in nim.cfg?

@jyapayne
Copy link
Contributor Author

I agree with you completely for the case of actually compiling the code. However, my use case is simply being able to specify the preprocessor for cross compiling to the Nintendo switch.

https://github.com/jyapayne/nim-libnx/blob/38c9efc2ab84e1f41c4a0730fe8e9cadd2d3ae4b/libnxGen.cfg#L3

@genotrance
Copy link
Owner

So are you changing nim.cfg to point to the aarch64-none-elf-gcc compiler?

@jyapayne
Copy link
Contributor Author

Nope, but I'm adding the cross compiler as an OS switch in the Nim compiler. nim-lang/Nim#8069

@jyapayne
Copy link
Contributor Author

I'm thinking something like an environment variable makes sense but it is kludgy. A nimgen global cfg file might also be an idea since everything would run through the same compiler in theory. Perhaps, nimgen should use the same compiler as specified in nim.cfg?

It looks like I ignored your question, my apologies. I would say that your idea of using the same compiler in the nim.cfg is a good idea! Env vars are kind of kludgy, but I do still use them from time to time.

@genotrance
Copy link
Owner

I did research this but doesn't seem trivial since I'll have to parse nim.cfg myself and get stuff out of extccomp. Unless Nim can tell me itself.

More important though, I will have no idea if the user uses a -os flag that changes the compiler to be used. Hope you have a better understanding of how it works to think of a way to pull this detection off.

@jyapayne
Copy link
Contributor Author

Yeah, you're right. It would be quite difficult to do this without leveraging the Nim compiler in some way to give us the parsed data from the nim.cfg.

I'm not really sure what the way forward is on this PR. The code I've submitted works for me because I'm cross compiling using aarch64-none-elf-gcc on Linux only, but it won't work for everyone like you mentioned. The simple way here is to just add an environment variable (or use CC/CXX by convention) and modify nimgen to use environment variables. It's kludgy but simple. What are your thoughts?

@genotrance
Copy link
Owner

So if we simplify this to CC/CXX/CPP, how will it help your use case? Nimgen will pick up the env vars but you will need to set them before installing the nimble package.

Does it really matter whether you use gcc or aarch...gcc for the preprocessing? I guess the #defines might change.

@jyapayne
Copy link
Contributor Author

jyapayne commented Jul 8, 2018

Does it really matter whether you use gcc or aarch...gcc for the preprocessing? I guess the #defines might change.

It matters a lot. Some of the files for libnx I'm processing have specific compiler values that need to be inserted while preprocessing that are specific to the Switch's architecture. Defines, macros, etc.

So if we simplify this to CC/CXX/CPP, how will it help your use case? Nimgen will pick up the env vars but you will need to set them before installing the nimble package.

It would still be feasible to use. I could set the env vars inside the .nimble file before using nimgen. In theory it should work.

@genotrance
Copy link
Owner

Ok then let's do the env var method. Are you okay with updating this PR?

@genotrance
Copy link
Owner

I think there's some confusion here - when I said env vars, I meant loading gCCompiler from the environment variable CC rather than the n.global section.

I'm cool with the ${} syntax along with ${output} but am wondering about staying consistent with the pipe syntax using $file. Should that be changed to ${file} as well?

It appears the ${} syntax is supported only in [n.include] and [n.exclude] sections. Do you see it useful elsewhere?

@jyapayne
Copy link
Contributor Author

jyapayne commented Jul 8, 2018

I think there's some confusion here - when I said env vars, I meant loading gCCompiler from the environment variable CC rather than the n.global section.

But then it's not clear where the env vars are coming from. The way I have it now makes it so that you can set cpp_compiler=${CXX}, which will tell the user where the compiler config is coming from. Sure, we could put it in the docs, but this way it's a little bit more self explanatory.

I'm cool with the ${} syntax along with ${output} but am wondering about staying consistent with the pipe syntax using $file. Should that be changed to ${file} as well?

That syntax is actually identical in terms of Nim's string interpolator from strutils. It supports either $identifier or ${identifier}. The latter is to make it so that the identifier is clear, otherwise $identifierbar will not replace $identifier, unless ${identifier}bar is used. This is useful for paths, for example.

It appears the ${} syntax is supported only in [n.include] and [n.exclude] sections. Do you see it useful elsewhere?

Not true, it is actually supported where ever you access a key from any of the configs. See this function

@genotrance
Copy link
Owner

I see what you are saying, just felt it was easier to change the compiler by simply setting an env var. If you have to set it in .cfg as well then every wrapper dev will have to be explicit in setting those values in n.global. If they didn't then you would have to edit the package and couldn't simply nimble install.

I feel we should change pipe to use ${file} as well then.

@jyapayne
Copy link
Contributor Author

jyapayne commented Jul 8, 2018

I feel we should change pipe to use ${file} as well then.

It already is this way :) Like I said, either syntax works because of how Nim's strutils.% operator is implemented. I could put a note in the README.md that says you can use either syntax because it isn't clear unless you know how the backend is implemented.

I see what you are saying, just felt it was easier to change the compiler by simply setting an env var. If you have to set it in .cfg as well then every wrapper dev will have to be explicit in setting those values in n.global. If they didn't then you would have to edit the package and couldn't simply nimble install.

With this change, you don't have to specify cpp_compiler or c_compiler, as the default will be gcc/g++. But I think I see what you're saying. What if we did this:

If cpp_compiler or c_compiler is set, use that value and any env vars referenced. If not, use CC/CXX/CPP. If those are not set, default to gcc/g++. Would that work?

@genotrance
Copy link
Owner

Yes, I'm good with that design - that way, we aren't forcing wrapper devs. to add that. So by default gcc, if c_compiler defined, use that. If envvar exists, load that.

I think the env var CC should have higher priority than the value defined in c_compiler. This way, a local overload has priority over a generic wrapper definition. What's your opinion on that?

@jyapayne
Copy link
Contributor Author

jyapayne commented Jul 8, 2018

I think the env var CC should have higher priority than the value defined in c_compiler. This way, a local overload has priority over a generic wrapper definition. What's your opinion on that?

I think it could be either way. For me, I prefer having the cfg file have highest priority because my libnx wrapper should not allow overrides, however your way makes sense from the perspective of the user of the wrapper because they may want to use a different compiler.

I think we should have a global value in the config called allow_compiler_override with a default value of true that allows the wrapper creator to allow CC/CPP/CXX to override the settings in n.global. Then we can have the priority you just said with the ability to disable it so that my project works as well. Is that good with you?

@genotrance
Copy link
Owner

I think allow_compiler_override will just be overkill. User should install and use packages as documented. If they are using your package, it is unlikely they will want to fiddle with the compiler anyway since it wouldn't work. They would only consider an override if the default was selected and they were wanting to do something custom.

With that logic, I think n.global:c_compiler should override ${CC} which would override default gcc.

README.md Outdated
output="src/path"

[n.include]
"${output}/library/include"
"${MY_INCLUDE_PATH}/include"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line here is why I need absolute directories (from the other PR). The nim-libnx port needs to reference third party includes installed on the user's system beforehand.

README.md Outdated
@@ -30,6 +30,17 @@ __Capabilities & Limitations__

Nimgen supports compiling in C/C++ sources and static libraries as well as loading in dynamic libraries.

Environment variables are supported via Nim's string interpolation `%` symbol imported from the `strutils` module. Simply use double quotes to enclose any value and put `$` or `${}` around the environment variable name. In addition, the `output` var from the n.global section is available as ${output}. For example:
Copy link
Owner

Choose a reason for hiding this comment

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

Can we move this section to the Confile File section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep!

README.md Outdated
[n.include]
"${output}/library/include"
"${MY_INCLUDE_PATH}/include"

To see examples of nimgen in action check out the following wrappers:-
Copy link
Owner

Choose a reason for hiding this comment

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

Can we add nim-libnx to this list as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually :) Right now I still need to file a couple PRs in order to get nim-libnx to work with vanilla nimgen.

nimgen.nim Outdated
CPP_COMPILER_ENV = "CPP"
DEFAULT_C_COMPILER = "gcc"
DEFAULT_CPP_COMPILER = "g++"

Copy link
Owner

Choose a reason for hiding this comment

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

Can we use camel case to adhere to nep1?

cCompilerEnv, etc. Shouldn't hurt to hard-code too since it isn't expected to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@genotrance
Copy link
Owner

Awesome work, appreciate the efforts!

@jyapayne
Copy link
Contributor Author

jyapayne commented Jul 9, 2018

Thank you for saying that! I appreciate you taking the time to review these as well. It helps a lot because you're well versed in making these changes cross platform to work for everyone.

@genotrance genotrance merged commit 84894bf into genotrance:master Jul 9, 2018
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.

2 participants