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

per_file support for flags #61

Open
samoht opened this issue Apr 26, 2017 · 25 comments
Open

per_file support for flags #61

samoht opened this issue Apr 26, 2017 · 25 comments

Comments

@samoht
Copy link
Member

samoht commented Apr 26, 2017

I would like to change the flags on a specific file (e.g. a generated file where I want to disable some warnings). Is this currently possible? If not, I think using something like this could be useful:

(flags (per_file (<flag> (module-list)))
@ghost
Copy link

ghost commented Apr 26, 2017

It's not currently possible, but adding support for per_file in the flags fields seems good

@bmillwood
Copy link

Interesting that you have it as a flag -> module list mapping, and not a module -> flag list one. I can imagine situations in which either thing could be the one you wanted (both are ultimately ways of concisely expressing the thing which matters, namely (flag * module) list)

@samoht
Copy link
Member Author

samoht commented Apr 26, 2017

@bmillwood my primary use-case is turning off some warnings on generated files, so something like:

(library
  ((name foo)
   (flags (per_file (:standard -w -32 (bar toto titi))
   ...

I guess it would also work in the other way round but would be a bit less convenient.

@mseri
Copy link
Member

mseri commented May 14, 2017

It would also be useful for packages like https://github.com/c-cube/ocaml-containers to enable the -nolabels option in all the *Labels.ml* files in a (relatively) compact way. Is there any way to mimick this behaviour at the moment?

@ghost ghost added this to the 1.1.0 milestone Jun 2, 2017
@aantron
Copy link
Collaborator

aantron commented Jun 28, 2017

This would also be useful for Lwt. For example, see ocsigen/lwt#354 (comment).

@rgrinberg
Copy link
Member

rgrinberg commented Oct 20, 2017

@bmillwood What you're suggesting sounds a lot like ocamlbuild's tags system. I don't have many fond memories of ocamlbuild, but I thought that how it let one concisely specify where extra command line flags are needed was one of its strong points.

@ghost ghost removed this from the 1.1.0 milestone Jun 2, 2018
@ygrek
Copy link

ygrek commented Aug 6, 2018

this seems essential for any big project that has code from various generators

@rgrinberg
Copy link
Member

While this feature is important, I'd actually recommend for the code generators to change to output correct warning removing annotations ([@@@ocaml.warning ".."]). This will work for all build systems, and will rid you having to maintain to maintain a separate set of warnings everywhere where you might use such a generator.

@ygrek
Copy link

ygrek commented Aug 6, 2018

Yes, this would be desirable, but

  1. existing world is not ideal, and some flexibility in build system would be helpful
  2. the problem still remains when compiler adds a new warning and generator is not updated or old version is used due to some other reasons

@ygrek
Copy link

ygrek commented Aug 7, 2018

One more use-case : enabling -opaque for generated version.ml (without introducing separate dev and release modes) for native binaries

@rgrinberg
Copy link
Member

By the way, the way that I'm currently thinking of solving this problem is allow glob/file constructors in the flags field. This will be consistent with how we'd like to extend the OSL elsewhere as well. @diml what do you think?

One more use-case : enabling -opaque for generated version.ml (without introducing separate dev and release modes) for native binaries

Note that with our current support for opaque, this actually will not do so much good as we'll still be generating the same rules. Would you expect dune to setup correct per modules based on the flags that you've provided to modules? This is possible, but has some limitations and isn't easy to do.

@ghost
Copy link

ghost commented Aug 8, 2018

By the way, the way that I'm currently thinking of solving this problem is allow glob/file constructors in the flags field. This will be consistent with how we'd like to extend the OSL elsewhere as well. @diml what do you think?

That seems good to me. Note that we can do these two features independently:

  • make flags fields support the per_module construct
  • extend all fields support the per_module construct to also support globs

BTW, @ygrek nobody is questioning the value of this feature. It's just a question of priority, all dune developers already have a packed todolist. If someone is willing to spend a bit of time on this, that would help a lot, even if the result is not a fully polished feature.

@rgrinberg
Copy link
Member

extend all fields support the per_module construct to also support globs

Oh, I was thinking to add the glob support directly to flags. So that you could do things like:

(flags (glob_files *.bar -w -1))

I'm not sure if the extra per_file is necessary. Although per_module might still be useful.

@ghost
Copy link

ghost commented Aug 8, 2018

Oh, I was thinking to add the glob support directly to flags

The two are not incompatible, glob_files is still a general construct that could apply to different kind of fields.

Regarding the syntax, I feel like there is something missing to separate the thing we are dispatching on and the arguments.

What about:

(flags
  (glob_files *.bar -> -w -1)
  (glob_files *.foo -> -w -1))

It feels like the term glob_files is not right as well. It's fine in dependency specifications, but here we really want to say: for all the files matched by this glob, use these flags and I'm not sure glob_files makes this clear.

@ygrek
Copy link

ygrek commented Aug 8, 2018

Would you expect dune to setup correct per modules based on the flags that you've provided to modules?

I feel I don't have enough understanding of some mechanics to figure what is the catch here compared to my description :) But I think it is important to have a way to tell dune to compile one file/module (version.ml in my case) with option -opaque. My understanding is that this will be enough for further build steps to detect that build artifacts (cmx in this case) didn't change and no recompilations "down the chain" are necessary.

BTW, @ygrek nobody is questioning the value of this feature. It's just a question of priority, all dune developers already have a packed todolist.

Ok, sure, it just looked abandoned (and removed from 1.1.0). It is up to developers to decide priorities based on bigger picture of course, my job as a user here is to provide some pressure/feedback for the priority-assignment algo :)

@ghost
Copy link

ghost commented Aug 9, 2018

My understanding is that this will be enough for further build steps to detect that build artifacts (cmx in this case) didn't change and no recompilations "down the chain" are necessary.

Yes, that's correct. More precisely, if you pass -opaque manually and dune doesn't know about it and version.ml is modified then the following will happen: .ml files that depend directly on Version will be rebuilt, but the resulting .cmx files will be unchanged. So this will cause some recompilation but it won't propagate.

@ygrek
Copy link

ygrek commented Aug 9, 2018

if you pass -opaque manually and dune doesn't know about it

is this theoretical scenario or it is possible to setup somehow with current dune release?

@ghost
Copy link

ghost commented Aug 13, 2018

I was referring to:

 (flags (:standard -opaque))

Technically, dune could see that you passed -opaque. However it's not easy to decide that in this case you are passing -opaque to the compiler but not in this case: (:standard -cclib -opaque). So dune doesn't try to be too clever here.

@ygrek
Copy link

ygrek commented Aug 13, 2018

Ah, it means for all files, and dune is agnostic to flags, so no rebuild takes place because of natural build artifact change detection, got it, thanks.

@rgrinberg
Copy link
Member

Now that we have the predicate_lang, I wonder if we need to extend it to describe maps rather than just sets. I can imagine this being useful not just for flags.

@ghost
Copy link

ghost commented Dec 11, 2018

That seems fine to me

@gares
Copy link
Contributor

gares commented Jun 19, 2019

+1 for this feature request

@SkySkimmer
Copy link

This would be useful for -rectypes: -rectypes when passed to a .ml but not to the .mli is supposed to work without requiring users to also pass -rectypes.

@eponier
Copy link
Contributor

eponier commented Apr 5, 2024

I'm interested in this too, for the same reason as the initial comment, i.e. to enable less strict warnings on some generated files. I understand that #2019 could be used to do that, but that it has never been connected to the rest of dune. Is this correct? How much effort would that be?

@toots
Copy link
Contributor

toots commented Jun 18, 2024

+1!

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

No branches or pull requests