search

snowleopard / hadrian

Hadrian: a new build system for the Glasgow Haskell Compiler. Now merged into the GHC tree!
https://gitlab.haskell.org/ghc/ghc/tree/master/hadrian
MIT License
208 stars 39 forks source link

Rules for rts/rts.cabal are broken #655

Open ndmitchell opened 5 years ago

ndmitchell commented 5 years ago

Two observations:

Seems like there's a few issues around there? This is on Mac with the --configure argument not set.

Reason for wanting to tweak rts.cabal.in after the fact was #614.

snowleopard commented 5 years ago

To produce rts.cabal from rts.cabal.in we need to run the configure script, but a few people demanded the corresponding rule to be locked behind the --configure flag.

So, this is not a bug but rather an unfortunate and bad (in my opinion) design decision. I'm happy to unlock the configure rule but regularly going back and forth on this is a bit silly. We should finally make this decision for good.

ndmitchell commented 5 years ago

I must say it's hard to distinguish this behaviour from a bug... And I guess people are also required to know when to rerun the configure script, and what inputs it takes? It seems a bit of a nightmare the way it currently is... Anyway, happy to close this as "expected" behaviour.

snowleopard commented 5 years ago

I agree that this looks and quacks like a bug.

@bgamari @hvr @angerman: I believe you all insisted on not allowing Hadrian to run the configure script automatically. What do you think of this issue?

I think for non-expert users who don't build GHC every day this will definitely be very confusing. On the other hand, expert users who know and use various special flags with the configure script can certainly learn to run Hadrian with the --skip-configure flag if they want to do the configuration step manually.

Mistuke commented 5 years ago

I'd just like to throw my hat in saying that I agree with @angerman , @hvr and @bgamari here.

I hate autoconf with a passion, but given that it's there I'd like it to work on the principle of least surprises. Every build system with autoconf I've ever used separates out configuration and building. I would find it very annoying if the build system reconfigured my tree without me asking.

Aside from it being unusual, I'm also not sure how you'd actually get it correct either. Would you be extracting the configuration flags from config.status to know how to re-invoke configure? How will you determine any environment variables I've set when I configured the tree that aren't there anymore.

When configure touches and regenerates other configuration files like ghcautoconf.h, will this trigger hadrian to do a full rebuild since this file touches almost everything?

The amount of ways this could go wrong is quite large, why open up that can of worms. It seems quite logical to me that if you make the conscious effort to change a configuration input file, you must manually reconfigure. You never know the state of my console or environment at the reconfigure time and must not assume it's the same as it originally was.

ndmitchell commented 5 years ago

@Mistuke how do you concretely suggest solving the problem at the top? How am I to know that changing rts/rts.cabal.in requires a configure? Or alternatively, should it? If we focus on the specific problem here, and how to make it better for me, so I don't screw up again :)

hvr commented 5 years ago

@ndmitchell do what I wanted to do for the makefile-based system already but refrained from (because Hadrian came along and made it seem like wasted effort): let the build-system perform the x.in -> x transformation, rather than the ./configure phase; let the ./configure phase dump all key/value pairs we'd be interested to @@-replace into an intermediate key/value mapping text-file, which is then used by the build-system to perform the .in-replacements.

ndmitchell commented 5 years ago

@hvr sounds good! If the configure output was purely an input (had no significant dependencies on in-source stuff) then it's much easier to do either way.

hvr commented 5 years ago

Are there any significant dependencies to in-source entities you're worried about?

Mistuke commented 5 years ago

@ndmitchell the typical way you solve this is having the build system compare the timestamps on the input and generated file and issue an appropriate warning.

Now whether you want this to be a warning or a hard abort can be a config flag, I assume Hadrian has an equivalent to build.mk. Where you can tell it to abort if you want.

ndmitchell commented 5 years ago

@hvr rts.cabal.in is the one that's tripped me thus far. The fewer the better!

@Mistuke that seems like threading a complex needle. If we go for what @hvr suggested we eliminate the problem at source.

snowleopard commented 5 years ago

I like @hvr's solution. Most of the @@ settings are already available to Hadrian via system.config produced by the configure script, so it's just a matter of adding a rule that does string replacement.

Mistuke commented 5 years ago

@hvr solution just moves the goal post, a person would then modify configure and wonder why the .in file wasn't changed. or modify configure.ac and wonder why configure didn't work.

You're hacking around a fundamental property of autoconf for very little gain.

I for one, would like an option to turn off this "feature" as I wouldn't trust it with a 10 foot pole.

hvr commented 5 years ago

@Mistuke is right, my suggested solution only addresses part of the problem; specifically the part where you want to optimize the cabal.in -> cabal transformation for whe you're just messing with the .cabal.in files and you don't touch the configure files; but it's not something that occurs very frequently either...

...and once you modify the configure scripting, hadrian still ought to warn you that the build might be outdated; so you still need to implement something as suggested by @Mistuke in https://github.com/snowleopard/hadrian/issues/655#issuecomment-413507126 regardless to tell the user they might need to re-run the configure phase.

snowleopard commented 5 years ago

Yes, I agree. The two solutions are not mutually exclusive. Implementing @hvr's is easier and solves this particular issue, so I don't see any harm in implementing it immediately.

@Mistuke's proposal to introduce warnings for outdated configure-generated files is useful too, but it's not obvious to me what the right implementation is, so perhaps it deserves a separate issue.

ndmitchell commented 5 years ago

@snowleopard I think having configure be optional per run and wired through the build system is a bit of a disaster. I'd have it before shake starts up, and just check that the timestamp of the configure output is newer than the timestamp of the configure input. The --configure flag really changes the build system itself, so which you might be able to come up with a crazy encoding, it's not going to be fun.

I'd also minimize the problem using @hvr's suggestion.