-
Notifications
You must be signed in to change notification settings - Fork 454
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
fix warnings and enable them again #1664
base: main
Are you sure you want to change the base?
Conversation
Actually, these warnings were always disabled (in the Makefile), mostly because they are so noisy. 27 in particular is an annoyance here, since it requires extra deviations when transliterating "paper rules". For 70, I can't see how the sources possibly compile under OCaml 4.13+ without it turned off; it was introduced after people ran into build failures (see #1479). I am open to enabling the others, if the changes aren't too tedious -- hard to see for me what they require, since the diff drowns in changes related to 27. ;) Could you perhaps create smaller PRs for the others? |
PS: Also, I think we want to keep the |
I tend to prefer having to add a
I tested on
Could you tell me why ?
All changes in the PR are related to warning 27. The vast majority is simply adding a |
Normally, yes. But here, where we try to transliterate paper rules, not so much. :)
Well, I suspect a reason, see last point below.
So that code with warnings is rejected, especially by CI, and cannot be checked in. Because that would just make the next person's life harder and confusing.
Hm, I just tried to reenable all warnings, in the Makefile of my local checkout, and got hundreds of warnings from the other disabled flags, too. Are you sure this PR actually fixes all warnings and dune isn't just swallowing them? Perhaps try reactivating -warn-error and see if it still builds from scratch. |
Can you give me an example of such warning you're getting ? I can run |
Here is what I get when I build HEAD with
|
Perhaps some of these warnings are off by default? Then this PR would likely deactivate more warnings than it activates, because it removes the Ah yes: "The default setting is -w +a-4-6-7-9-27-29-32..42-44-45-48-50-60." (manual) |
Dune has its own set of default warnings which is not the same than the default choosen by the compiler. Dune also turns warnings to error by default. |
Also I don't understand, why is the Makefile still calling ocamlbuild ? |
I see. So effectively, this PR turns on 27, but also turns off a whole set of other warnings.
Because nobody changed that. :) |
OK, I'll do a first PR where we use dune everywhere... It'll avoid this kind of confusing stuff. |
One reason for holding back on making dune obligatory was its lack of proper Windows support. What's the status of that nowadays? |
Dune is working fine on Windows (and already was at the time). I made a first PR to add dune support where I've been asked to generate some script to build without dune on windows (which I'm not interested in so I didn't took the time to do it). Then someone else made another PR adding dune support (which is what I did initially) and this one was merged but kept ocamlbuild in the Makefile and ocamlbuild to generate the windows script. So now we've got three ways to build the interpreter:
I would like to remove (2). I'll keep ocamlbuild only to generate (3) (or we could just remove (3) completely, dune is not that hard to install on windows...). |
Ah, sorry about that. There is some history and politics around this... Yeah, at this point I highly doubt anybody is still using the winmake file, so I think we can risk killing it. We certainly don't want to remove the Makefile, which is an abstraction for a whole lot of things. But switching the build targets over from invoking ocamlbuild to invoking dune sounds good to me. For the transition, it would be good to make the Makefile probe first whether dune is installed, and if not, print an understandable message pointing to updated install instructions. So that non-OCaml experts on whom the build suddenly starts failing know what to do. |
Cool! Will do.
I'll do. Note that ocamlbuild is already not distributed with ocaml anymore - it's been a few years. People already have to install it manually. |
I know, but everybody building Wasm of course has it installed right now, but not necessarily dune. I have been burnt many times by software that used to build fine but suddenly refuses to, with no clue what's going on because the developers somehow expect everybody to be familiar with their niche tool chain and random dependency changes. |
@zapashcanon, do you still plan to pursue this? |
Yes, I'll try to update the PR soon now that the other one has been merged, thanks for the reminder |
a808782
to
92acc13
Compare
@rossberg, I updated the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this still is super-intrusive. I'm afraid that means that we can land it as long as we have a substantial number of proposal forks out there, since it will cause conflicts all over the place in most of them. Maybe after all Wasm3 proposals have been merged there is a more realistic opportunity.
(I'm also still somewhat unhappy how this visually moves the code further away from the paper rules. Though maybe I can live with that eventually...)
(env | ||
(_ | ||
(flags | ||
(-w +a-4-27-42-44-45-70 -warn-error +a-3)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for removing 27, I think we still want to keep this declaration for stricter checking? The default is much more lax, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default of the OCaml compiler are more lax. Dune has its own set of warnings which is much more stricter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not stricter than this, AFAICT.
Hi,
A lot of default warnings were disabled in #1459 (I guess it was mainly to avoid having to make the various changes I'm doing now). It is a pain (for me at least) to hack the interpreter without them as they usually prevent mistakes. I enabled all warnings again an fixed the code accordingly.