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 static_c_args to library() #3304

Closed
xclaesse opened this issue Mar 23, 2018 · 31 comments
Closed

Add static_c_args to library() #3304

xclaesse opened this issue Mar 23, 2018 · 31 comments

Comments

@xclaesse
Copy link
Member

It is common to pass different cflags depending if it's a static or shared library, for example -DSTATIC_COMPILATION.

I suggest adding static_c_args that will be appended to c_args if it's a static build. If static_c_args is not empty and default_library=='both' then it will have to build twice instead of extracting objects.

xclaesse added a commit to xclaesse/meson that referenced this issue Mar 24, 2018
It defines extra cflags to pass only when building the static version of
the library. When building both shared and static libraries and
static_c_args is not empty, it will build the library twice instead of
extracting objects.

Closes mesonbuild#3304
@sarum9in
Copy link
Contributor

I believe static makes sense for multiple languages, e.g. C++
So if we are to implement this, we need to support more generic form.
Do we also need this for pch?

<languagename>_pch precompiled header file to use for the given language
<languagename>_args compiler flags to use for the given language; eg: cpp_args for C++
++ static_<languagename>_args

@xclaesse
Copy link
Member Author

The patch I made adds for c and cpp because I have real use case for them. What other language would need it?

@jpakkane
Copy link
Member

Fortran, objc, Rust, D, and everything that compiles into native code. It could also be argued that you should make it possible to add args to shared libs and exes as well. Possibly also based on the build type. This gives #languages x #targettypes #buildtypes new kwarg parameters. This is not a feasible solution, since we try to keep the number of keyword arguments as low as possible.

@xclaesse
Copy link
Member Author

Well, for static_c_args there are valid real world use-case: glib and gstreamer do have a -DSTATIC_COMPILATION to define dllexport on windows which is different for static/shared.

We could have static_args : {'c' : '-DSTUFF'}, but I'm not sure the interpreter supports dict, does it?

For shared_args I have no use case for it, but if there are, I don't think it's a big deal to add it later.

@xclaesse
Copy link
Member Author

xclaesse commented Mar 25, 2018

Just got an idea to make it scalable: Add a new type CompileArgs. Something like this:

args = compile_args(static_args : '-DSTATIC', shared_args : '-DSHARED')
lib = library(..., c_args : ['-DFOO', args])

Making existing <lang>_args accept a new generic type makes it extensible in the future. What do you think?

Note: IMO the proposed static_<lang>_args is better, just proposing alternatives in the case you think otherwise. Also as long as we don't have real world use-case, I don't see the need of adding static_pch or shared_c_args or static_c_args on executable/shared_library, or any other variants.

@jpakkane
Copy link
Member

Or perhaps mirroring how configure_file works:

cargs = compile_args()
cargs.add_args('-foo', language: 'c', targettype : 'static_library')
library(.., extra_args : cargs)

?

xclaesse added a commit to xclaesse/meson that referenced this issue Mar 26, 2018
The principal use-case is to pass different compiler flags to library()
depending if it's building the static or shared library.

Closes: mesonbuild#3304
@xclaesse
Copy link
Member Author

Actually I like that idea of having an holder for all the build configs that can be passed to many library() executable() calls. It can replace add_global_arguments() for args that needs to be on most but not all targets.

It could also be extended to more than compiler args later. For example why not args.set_install_dir('/myprefix/lib', target_type: 'library'). All your settings could be in one place and you pass that object down to all your build targets. Anyway, that's for later if we see the need. For now the real use-case for me is static c flags.

xclaesse added a commit to xclaesse/meson that referenced this issue Mar 26, 2018
The principal use-case is to pass different compiler flags to library()
depending if it's building the static or shared library.

Closes: mesonbuild#3304
xclaesse added a commit to xclaesse/meson that referenced this issue Mar 28, 2018
The principal use-case is to pass different compiler flags to library()
depending if it's building the static or shared library.

Closes: mesonbuild#3304
xclaesse added a commit to xclaesse/meson that referenced this issue Apr 4, 2018
The principal use-case is to pass different compiler flags to library()
depending if it's building the static or shared library.

Closes: mesonbuild#3304
@nirbheek
Copy link
Member

I wonder if people need to pass arbitrary custom <lang>_args when doing a static compilation, or only defines. If the latter, the implementation is much simpler because it applies to C/C++/ObjC/ObjCpp at once.

@NickeZ
Copy link
Contributor

NickeZ commented Jul 25, 2018

I would argue that special defines for building dynamic libs is often the case on windows.

Sometimes it is even necessary to have special defines only when building with the intention of linking dynamically. Real world examples: hdf5 has to set H5_BUILT_AS_DYNAMIC_LIB and boost has to set BOOST_ALL_DYN_LINK.

#ifdef _WIN32
#  ifdef MODULE_API_EXPORTS
#    define MODULE_API __declspec(dllexport)
#  else
#    define MODULE_API __declspec(dllimport)
#  endif
#else
#  define MODULE_API
#endif

Source: https://stackoverflow.com/a/19547601/3804521

One could also argue that -fvisibility=hidden is a good practice using gcc/clang and then the above macro becomes something like this:

#if defined _WIN32 || defined __CYGWIN__
  #ifdef BUILDING_DLL
    #ifdef __GNUC__
      #define DLL_PUBLIC __attribute__ ((dllexport))
    #else
      #define DLL_PUBLIC __declspec(dllexport) // Note: actually gcc seems to also supports this syntax.
    #endif
  #else
    #ifdef __GNUC__
      #define DLL_PUBLIC __attribute__ ((dllimport))
    #else
      #define DLL_PUBLIC __declspec(dllimport) // Note: actually gcc seems to also supports this syntax.
    #endif
  #endif
  #define DLL_LOCAL
#else
  #if __GNUC__ >= 4
    #define DLL_PUBLIC __attribute__ ((visibility ("default")))
    #define DLL_LOCAL  __attribute__ ((visibility ("hidden")))
  #else
    #define DLL_PUBLIC
    #define DLL_LOCAL
  #endif
#endif

Source: https://gcc.gnu.org/wiki/Visibility

In both cases either MODULE_API_EXPORTS or BUILDING_DLL has to be set while building the dynamic version of a library on windows.

@nirbheek
Copy link
Member

Exactly, I've only ever seen people need to pass special defines on Windows when doing static/dynamic linking, and the implementation for that is much simpler than special arguments.

@NickeZ
Copy link
Contributor

NickeZ commented Jul 25, 2018

Maybe I'm going off-topic but: I think an abstraction involving defines would be helpful. It was not immediately obvious to me that -DH5_BUILT_AS_DYNAMIC_LIB would be passed as /DH5_BUILT_AS_DYNAMIC_LIB on windows when I started using meson. Having some sort of abstraction that would let me write add_project_define('H5_BUILT_AS_DYNAMIC_LIB') or add_project_define('LESS=MORE') would be nice. Of course I would still need something like in the scope of this issue where I could have different defines depending on the target type I'm building and the library type I'm intending to link against.

@xclaesse
Copy link
Member Author

@nirbheek FWIW, my only use-case is glib/gstreamer's -DSTATIC_COMPILATION. IMHO it's a big enough use-case in itself to justify having a solution just for that, and that's why my initial PR wasn't a generic solution. But @jpakkane wanted to have something more generic.

@nirbheek
Copy link
Member

nirbheek commented Jul 25, 2018

It was not immediately obvious to me that -DH5_BUILT_AS_DYNAMIC_LIB would be passed as /DH5_BUILT_AS_DYNAMIC_LIB on windows when I started using meson

fwiw, /D and -D are identical for cl.exe, and we pass that option unchanged. It's the same for all /FOO options passed to cl.exe, link.exe, and lib.exe. You're right, we should have API for defines outside of <lang>_args.

@xclaesse I think we can probably add that generic mechanism when people ask for it, but it's better to implement a feature with smaller scope first.

I would actually imagine this being implemented as:

library('foo', 'foo.c',
        defines: {'static': ['FOO=bar', 'BAR=baz'],
                  'shared': ['BAH=blah']}

and then the defines get added to all possible languages. @jpakkane what do you think?

@jpakkane
Copy link
Member

NO new kwargs only for defines. -D is standard, works with every compiler and is something people already know and use.

@xclaesse
Copy link
Member Author

What about:

library('foo', 'foo.c',
  static_args : {'c' : ['-DFOO'],
                        'cpp': ['-DBAR']}
)

@xclaesse
Copy link
Member Author

xclaesse commented Jul 25, 2018

Or

library('foo', 'foo.c',
  c_args : {'static' : ['-DFOO'],
                 'shared': ['-DBAR']}
)

I actually prefer this one, because it doesn't introduce new kwarg, it just extend the value types we can pass to it. If it's a dictionnary, the key is the targettype.

@xclaesse
Copy link
Member Author

Could be used to group all args into a common dict that can be passed to any target.

cargs = {'executable' : ['-DEXE'],
              'shared_library' : ['-DSHARED'],
              'static_library' : ['-DSTATIC']}
executable('app', 'app.c', c_args : cargs)
library('lib', 'lib.c', c_args : cargs)

@NickeZ
Copy link
Contributor

NickeZ commented Jul 25, 2018

I like the last one. I think it would be cool if it was supported on:

cargs = {'shared_library' : ['-DH5_BUILT_AS_DYNAMIC_LIB']}
lib = library('lib', 'lib.c')
dep = declare_dependency(link_with: lib, compile_args: cargs)
executable('app', 'app.c', dependencies: dep) # <-- should get -DH5_BUILT_AS_DYNAMIC_LIB if lib was built/found as dynamic lib.

Unless I completely missunderstood the compile_args kwarg. I haven't actually used it like this yet.

@nirbheek
Copy link
Member

NO new kwargs only for defines. -D is standard, works with every compiler and is something people already know and use.

The advantage of new kwargs for defines would be to have them apply to all languages at once, and not have to worry about setting it individually for each language. For instance, in the use-case that @NickeZ posted above, the -D define would only work when executable() uses C sources. With a new option for defines, it would work with C/C++/Objc/Objcpp, and perhaps also D, Rust, Swift.

@NickeZ
Copy link
Contributor

NickeZ commented Jul 26, 2018

AFAIK rustc doesn't really have defines but they have --cfg which is used to enable/disable features. They are supposed to be "additive", i.e. setting a feature shouldn't disable something else.

Maybe some abstraction could be created that maps to all languages..

https://doc.rust-lang.org/reference/attributes.html#conditional-compilation

https://doc.rust-lang.org/book/first-edition/conditional-compilation.html

xclaesse added a commit to xclaesse/meson that referenced this issue Sep 11, 2018
The dict must map a target type to a list of compiler args. This makes
possible to pass different c_args to static and shared libraries when
using both_libraries(). In that case sources will be compiled twice.

Closes mesonbuild#3304.
xclaesse added a commit to xclaesse/meson that referenced this issue Sep 11, 2018
The dict must map a target type to a list of compiler args. This makes
possible to pass different c_args to static and shared libraries when
using both_libraries(). In that case sources will be compiled twice.

Closes mesonbuild#3304.
xclaesse added a commit to xclaesse/meson that referenced this issue Nov 2, 2018
The dict must map a target type to a list of compiler args. This makes
possible to pass different c_args to static and shared libraries when
using both_libraries(). In that case sources will be compiled twice.

Closes mesonbuild#3304.
xclaesse added a commit to xclaesse/meson that referenced this issue Nov 9, 2018
The dict must map a target type to a list of compiler args. This makes
possible to pass different c_args to static and shared libraries when
using both_libraries(). In that case sources will be compiled twice.

Closes mesonbuild#3304.
@nioncode
Copy link
Contributor

nioncode commented Jan 1, 2019

For the method in #3304 (comment) to work with dllimport / dllexport on Windows, I think there must be a way to differentiate between defines that are used when the library gets compiled and ones that get used when it is used as a dependency.

@jpakkane
Copy link
Member

jpakkane commented Jan 1, 2019

Any flags specified in the library call are only used when the library is being compiled. Flags that should be used when consuming the library should be put in a declare_dependency.

@nioncode
Copy link
Contributor

nioncode commented Jan 1, 2019

Ah true, then this would probably work quite nicely.

@tristan957
Copy link
Contributor

@xclaesse you said earlier in this thread you had a patch for this around 2 years ago. Any thoughts on picking that up? Or I could potentially if your old work no longer applies/don't have time.

Seems like the dictionary version of <language>_args is preferred.

@xclaesse
Copy link
Member Author

@tristan957 other developers did not like the dictionary approach. The last suggested idea was to do that with add_project_arguments(): #4159 (comment).

@dcbaker
Copy link
Member

dcbaker commented Mar 19, 2021

Let me add to this mess. Rust has several additional library (crate) types : c static, c dynamic, rust static, and rust dynamic. So we have at least two additional options here that I need today. So, I'd be in favor of the dictionary, but in a flat List<str> | Dict<str, List<str>> way. ie:

library(
  c_args : {
     'common': ['foo', 'bar'],
     'static' : '-DSTATIC',
     'shared' : '-DSHARED',
  },
  cpp_args : 'foo',
)

And for rust that could like like:

library(
  rust_args : {
    'common' : ['--cfg', 'always'],
    'rlib': ['--cfg', 'opt'],  # rust static
    'static': ['--cfg', 'static'],
    'cdylib': ['--cfg', 'dylib'],
  }
)

@dcbaker
Copy link
Member

dcbaker commented Mar 19, 2021

I'm at this point we need to agree to paint the dang shed, even if it's not perfect because this is starting to be a real problem.

@bruchar1
Copy link
Member

bruchar1 commented Mar 6, 2023

I'd be in favor of [<target_type>_]<lang>_args (and [<target_type>_]link_args, like the initial proposal, as it keeps the syntax simpler, and it avoids the need of an artificial all or common target type.

I'm also in favor of having a target_type argument to add_project_arguments and add_project_link_arguments. I think both approaches are complementary.

@xclaesse
Copy link
Member Author

xclaesse commented Mar 6, 2023

but in a flat List<str> | Dict<str, List<str>>

That's what I suggested 4 yearts ago too: #3304 (comment). Not sure we need common, as you can easily have a variable for that and do {'static': common + []}, but that's just a detail. IMHO we should just go ahead with that dict.

xclaesse added a commit to xclaesse/meson that referenced this issue Mar 6, 2023
This allows having different cflags for static and shared library when
using both_libraries().

Fixes: mesonbuild#3304
@xclaesse
Copy link
Member Author

xclaesse commented Mar 6, 2023

Let's just do it: #11501

@bruchar1
Copy link
Member

Can this be closed, now that #11981 is merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants