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

WIP: Add support for show-build-info json format #71

Closed
wants to merge 1 commit into from

Conversation

DanielG
Copy link
Contributor

@DanielG DanielG commented Feb 13, 2021

Found this in my local repo, looks like I never ended up submitting this :0

This adds support for parsing the json output of the lib:Cabal show-build-info command. See Distribution.Simple.ShowBuildInfo module (docs

@phadej
Copy link
Collaborator

phadej commented Feb 13, 2021

I think you are better off with creating your own library for now, e.g. cabal-show-build-info (or something). cabal-plan is quite done, show-build-info isn't.

@phadej phadej closed this Feb 13, 2021
@DanielG
Copy link
Contributor Author

DanielG commented Feb 13, 2021

I'm sorry but this makes zero sense. Since s-b-i very much is part of cabal's json interface and this library deals with interfacing with that why should this go into a different library? I have to say I'm not happy with your conduct in this matter. It feels like a knee-jerk reaction to me.

Who is currently officially maintaining this library anyway it was my understanding @hvr is still the maintainer and just shutting down this PR without even so much as letting it gather his/others input seems inappropriate to me.

@phadej
Copy link
Collaborator

phadej commented Feb 14, 2021

See https://github.com/haskell-hvr/cabal-plan/commits/master

It is json interface yes, but it is not plan.json.

My reaction is triggered by the fact that show-build-info is not documented. plan.json isn't either, and having more things to read about from the code is not nice for me as maintainer of this library.

I also consider show-build-info an experimental feature. With cabal-install-3.4:

% cabal show-build-info
cabal: unrecognised command: show-build-info (try --help)

Apparently it is only a Setup command which doesn't fit well with plan.json, which is cabal-install addon.

In particular, if you see that show-build-info output parser would be useful for stack, this is very wrong place to put it.

@DanielG
Copy link
Contributor Author

DanielG commented Feb 14, 2021

My reaction is triggered by the fact that show-build-info is not documented.

I mean that's just not true, I linked to the docs in lib:Cabal above. If you want need more docs I'm happy to contribute those but that's not going to happen by just shutting this down instead of offering constructive criticism.

If you're unhappy with the whole s-b-i paradigm why didn't you bring this up as part of the review process in Cabal? You were part of that process after all in haskell/cabal#6108.

I also consider show-build-info an experimental feature. [...] Apparently it is only a Setup command

cabal act-as-setup -- show-build-info --builddir <distdir>

Has been part of cabal-install since 3.2 now I believe and that's what this change is primarily concerned with. I don't think there's a good argument to be made that Setup.hs commands aren't a public interface considering that distributions such as Debian still call them directly for packaging purposes.

which doesn't fit well with plan.json, which is cabal-install addon

I mean the divide between cabal-install and lib:Cabal is entirely artificial why should json coming from one or the other be a valid deciding factor for inclusion in this package? I can re-use a number of data types and json parsers from Cabal.Plan in this module, the overall functioning is the same too. How is this not a good fit?

output parser would be useful for stack

Stack uses lib:Cabal too so the argument that because stack needs this code too it shouldn't go into a "cabal-*" library doesn't make sense to me either. In fact it only serves to reinforce the need to put this in a common library if anything.

That being said I do think that stack wouldn't actually require a parser for this format as it's pretty much write only for the build tool itself.


Look, Oleg, if you feel overworked as the maintainer of this package I'm happy to offer my help. I'm very sympathetic to the thankless work of being a FLOSS maintainer but at the same time this sort of unwelcoming public response isn't going to get more people interested in contributing either.

If this package is truly "done" and you're not interested in accepting any enhancements but only fixes, then why not put this fact prominently in the README so the community, i.e. me in this case, can release a non-hostile fork. Though I still think just helping with maintenance would be a better path forward.

@phadej
Copy link
Collaborator

phadej commented Feb 14, 2021

This is cabal-install companion library, not Cabal.

lib:Cabal and cabal-install are separate "projects", to me almost as separate as lib:Cabal and stack. And I would like that separation to made even clearer. (Some people disagree and would like the Cabal:lib and cabal-install:exe to be merged together, let's discuss when that happens).

@phadej
Copy link
Collaborator

phadej commented Feb 14, 2021

For the record, I was arguing that show-build-info command should only be in cabal-install. But it was added to Cabal, and not to cabal-install. Fix that and we can discuss this change again then.

@DanielG
Copy link
Contributor Author

DanielG commented Feb 14, 2021

For the record, I was arguing that show-build-info command should only be in cabal-install. But it was added to Cabal, and not to cabal-install. Fix that and we can discuss this change again then.

I'm sorrry, but architecturally that just doesn't make any sense. In order to read the info we need in s-b-i it has to be in lib:Cabal, how else are you proposing we read stuff from LocalBuildInfo?


If you're in favor of there being a hard split between cabal the build system and cabal-install what's your thoughts on cabal-install insisting that only lib:Cabal may implement the Setup.hs interface? Currently cabal-install has code that prevents any other library name in the setup-config header even though at the inception of the "The Haskell Cabal" specification it was once intended that a number of libraries could implement this interface.

My understanding is the reason for that is the whole backwards compatibility mess, see filterConfigureFlags and friends. How do you propose we fix those architectural problem if you want to de-couple lib:Cabal from cabal-install?

My opinion is that they should be considered one entity precisely because of this rather intricate interface between them.

@phadej
Copy link
Collaborator

phadej commented Feb 14, 2021

I'm sorrry, but architecturally that just doesn't make any sense. In order to read the info we need in s-b-i it has to be in lib:Cabal, how else are you proposing we read stuff from LocalBuildInfo?

Abandon build-type: Custom, use Cabal as a library to read it.

If you're in favor of there being a hard split between cabal the build system and cabal-install what's your thoughts on cabal-install insisting that only lib:Cabal may implement the Setup.hs interface?

Again, abandon build-type: Custom, then cabal-install would use Cabal as a library.


TL;DR having build-type: Custom escape hatch prevents any kind of alternative thing to emerging.
It's perfectly valid to use Cabal library to only parse the .cabal files but build the packages somehow differently. GHCs hadrian does exactly that.

Then, Cabal can be split into "how to parse .cabal files" and "how to build this packages" parts, where latter depends on the former but can be swapped. Then building part would reside completely in cabal-install, and there is no problems reading LocalBuildInfo. It is a build tool, it kind of knows that data already... except for build-type: Custom packages.

Initially people thought that having "you can do about everything" escape hatch would be good, but IMO it turned out to be very bad idea.

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