Skip to content

Add Distribution.Client.Main #8793

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

Merged
merged 2 commits into from
Feb 26, 2023
Merged

Add Distribution.Client.Main #8793

merged 2 commits into from
Feb 26, 2023

Conversation

sol
Copy link
Member

@sol sol commented Feb 21, 2023

This makes it easier to integrate cabal-install into other tools.

I need this for a functional scripting environment that uses cabal-install for dependency caching.

@ulysses4ever
Copy link
Collaborator

Some bikeshedding here: maybe instead of the driver directory we should use app, as we suggest in the skeleton created by cabal init. That would go more in the spirit of practice-what-you-preach.

@sol
Copy link
Member Author

sol commented Feb 22, 2023

Some bikeshedding here: maybe instead of the driver directory we should use app, as we suggest in the skeleton created by cabal init. That would go more in the spirit of practice-what-you-preach.

Done.

Though personally, I think driver is a better name in that it makes it very clear that you should not put any substantial amount of code in there (keeping everything easily unit testable and such). That's what I'm going to continue to use for my projects at least.

@andreasabel
Copy link
Member

To add my 2 cents: I think main (as it used to be) is preferable to app or driver...
app has the connotation of something running on a mobile phone or distributed via the App Store, something graphical...
main is also the name of the main function in Haskell programs.

@andreasabel andreasabel added re: API Concerning the Cabal API design and removed attention: needs-review labels Feb 22, 2023
@andreasabel
Copy link
Member

@sol: Whey you are ready, you can apply the merge me label (no squashing needed as this is a single commit).

@robx
Copy link
Collaborator

robx commented Feb 22, 2023

To add my 2 cents: I think main (as it used to be) is preferable to app or driver...

I tend to agree with this, if just to keep the change minimal -- I don't think either app or driver are an improvement and then why change things.

@sol
Copy link
Member Author

sol commented Feb 23, 2023

I tend to agree with this, if just to keep the change minimal

I wholeheartedly agree with this. The reason I changed the name in the first place is that otherwise git's rename detection will not do a very good job. While git log --follow src/Distribution/Client/Main.hs will still pick up the whole history, git blame will not. Though apparently, a user can try to get more useful results by passing -C at least twice (!!) to git blame.

If we are ok to split this into two commits (that we then should not squash) then I can do something that meets all of these points:

  1. Keep the history of src/Distribution/Client/Main.hs intact
  2. Have the driver stub in main/Main.hs
  3. Have each of these two commits in a buildable state so to not break git bisect

If we want this in one commit then I will go with driver/Main.hs (for reason along the line of what @andreasabel said). Alternatively I would be ok with main/cabal.hs.

Finally, let's not forget that at this point we drifted very deep into the realm of bikeshedding:

  • It's not entirely unlikely that this simple driver file will never be touched again. Ideally we would not need it at all (I'm not going to elaborate on this at this point, but GHC's -main-is, while promising, is too limited here).
  • Yes, this is a simple change and by extension lends itself to bikeshedding. Let's still try to keep this productive and not fall back into historic patterns (Pull request: Add the 'fork' command to cabal-install. #2).

TL;DR: Options:

  1. Go with driver/Main.hs (this PR) 👍
  2. Go with main/cabal.hs (requires one commit) 😄
  3. Keep main/Main.hs (requires two commits) 👀

@sol
Copy link
Member Author

sol commented Feb 23, 2023

@andreasabel apparently I don't have the permissions to add labels. From my side this is ready to merge. Option (3) could be done as a follow-up PR if somebody is inclined to do so.

@andreasabel
Copy link
Member

@sol : You should have the permissions now, I added you as collaborator.

I am still for option 3 (a second commit renaming driver back to main with the respective update in the cabal file).

@sol
Copy link
Member Author

sol commented Feb 24, 2023

I am still for option 3 (a second commit renaming driver back to main with the respective update in the cabal file).

Done.

@sol sol added the merge me Tell Mergify Bot to merge label Feb 24, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 26, 2023
@mergify
Copy link
Contributor

mergify bot commented Feb 26, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify mergify bot merged commit 8ec30a0 into haskell:master Feb 26, 2023
@sol
Copy link
Member Author

sol commented Feb 26, 2023

@andreasabel Will this make its way into 3.10? Any additional steps that I need to take?

@andreasabel
Copy link
Member

@Mikolaj : Will this go into 3.10?

@Mikolaj
Copy link
Member

Mikolaj commented Mar 3, 2023

Unfortunately, rather not into 3.10.1.0 --- we are past the hard freeze and I'm now struggling (and losing) with the changelogs. 3.10.2.0 is a possibility, though, if it happens.

@andreasabel
Copy link
Member

Ok, I see. Keep up the good fight, @Mikolaj !

@sol sol deleted the client-main branch May 17, 2023 03:18
@Kleidukos
Copy link
Member

@Mergifyio backport 3.10

@mergify
Copy link
Contributor

mergify bot commented Jun 17, 2023

backport 3.10

✅ Backports have been created

mergify bot added a commit that referenced this pull request Jun 17, 2023
Add `Distribution.Client.Main` (backport #8793)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge re: API Concerning the Cabal API design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants