Skip to content

Remove v2-prefix from other documentation where appropriate #9090

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 11, 2023
Merged

Remove v2-prefix from other documentation where appropriate #9090

merged 1 commit into from
Jul 11, 2023

Conversation

AphonicChaos
Copy link
Contributor

@AphonicChaos AphonicChaos commented Jul 6, 2023 •
edited
Loading

Note: I'm aware of @tomjaguarpaw 's PR here and intend to rebase once that lands as not to duplicate their efforts.

The general approach I took here was to search for any file that mentioned "v2-" that wasn't a test file and remove the prefix if:

  • it appeared to be a recommendation for what command to run
  • didn't appear to be commenting on the aliased commands

Happy to consider any cases that I've missed or revert anything that does what we don't want. My goal at the end of this is to not have cabal mention v2 anywhere it's not appropriate, whether that's in command line output, documentation, or otherwise. I'm happy to update tests if that's what's required.

I intend to complete this work (and thus properly fill out the checkbox below) over this weekend (July 7 - July 9, 2023).

Please include the following checklist in your PR:

Bonus points for added automated tests!

QA Notes

This PR isn't intended to change any functional behavior. Visually, I would expect the following:

  • the commands echoed by make targets should not display any v2- commands
  • none of the cabal dev scripts should print usage information that suggests v2- commands
  • benchmarks should not use v2- commands

@AphonicChaos
Copy link
Contributor Author

I also did a brief search of issues, and found the following:

Sorry if this is noise. Just wanted to be clear about the scope I'm targeting with this humble contribution.

@AphonicChaos AphonicChaos marked this pull request as ready for review July 6, 2023 09:26
@AphonicChaos
Copy link
Contributor Author

Finished reading contributor documentation, so I'm happy to remove this from draft, though I expect to implement feedback.

ulysses4ever
ulysses4ever reviewed Jul 6, 2023
View reviewed changes
CONTRIBUTING.md Outdated
@@ -4,17 +4,17 @@ Building Cabal for hacking
--------------------------

The current recommended way of developing Cabal is to use the
`v2-build` feature which [shipped in cabal-install-1.24](http://blog.ezyang.com/2016/05/announcing-cabal-new-build-nix-style-local-builds/).
`build` feature which [shipped in cabal-install-1.24](http://blog.ezyang.com/2016/05/announcing-cabal-new-build-nix-style-local-builds/).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be cut down further: no need to mention a) that build is a feature (sounds weird: a build tool like cabal is supposed to have such "feature"!), b) when it was released, c) link to the blog post.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, maybe strike the whole sentence?..

ulysses4ever
ulysses4ever reviewed Jul 6, 2023
View reviewed changes
ulysses4ever
ulysses4ever reviewed Jul 6, 2023
View reviewed changes
Cabal-tests/tests/ParserTests/regressions/hasktorch.cabal Outdated
@@ -15,7 +15,7 @@ homepage: https://github.com/hasktorch/hasktorch#readme
bug-reports: https://github.com/hasktorch/hasktorch/issues
synopsis: Torch for tensors and neural networks in Haskell
description:
Hasktorch is a library for tensors and neural networks in Haskell. It is an independent open source community project which leverages the core C libraries shared by Torch and PyTorch. This library leverages @cabal v2-build@ and @backpack@. *Note that this project is in early development and should only be used by contributing developers. Expect substantial changes to the library API as it evolves. Contributions and PRs are welcome (see details on github).*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest leaving tests alone: they're taken from real packages and I'd prefer not to touch them at all.

ulysses4ever
ulysses4ever reviewed Jul 6, 2023
View reviewed changes
ulysses4ever
ulysses4ever reviewed Jul 6, 2023
View reviewed changes
ulysses4ever
ulysses4ever reviewed Jul 6, 2023
View reviewed changes
doc/nix-local-build.rst Outdated
@@ -11,19 +11,19 @@ local builds with this command (configuring is not necessary):

::

$ cabal v2-build
$ cabal build
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should strike any changes to this file altogether in the light of the Tom's PR. This will make reviewers' live easier (less files to look at). And also will ease rebasing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. I was originally going to fork from his branch but didn't want to juggle multiple the remotes. Didn't even occur to me to just ignore file names he touched. Thanks

ulysses4ever
ulysses4ever reviewed Jul 6, 2023
View reviewed changes
Cabal/src/Distribution/Simple/Setup/Haddock.hs Outdated
@@ -442,7 +442,7 @@ defaultHaddockProjectFlags =
haddockProjectCommand :: CommandUI HaddockProjectFlags
haddockProjectCommand =
CommandUI
{ commandName = "v2-haddock-project"
{ commandName = "haddock-project"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think CI fails because of this change. Basically, v2- has semantic meaning in the Cabal library.

@AphonicChaos
Copy link
Contributor Author

Thanks for the feedback @ulysses4ever . I'll address these tomorrow

@ulysses4ever
Copy link
Collaborator

Thank you! Could you also take care of .github/ISSUE_TEMPLATE/bug_report.md? It needs removal of v2- and the whole sentence:

Please use version-prefixed commands (e.g. v2-build or v1-build) to avoid ambiguity.

@AphonicChaos AphonicChaos force-pushed the remove-v2-from-docs branch from 7a5bf67 to 1f49574 Compare July 7, 2023 16:59
@AphonicChaos
Copy link
Contributor Author

Alright, @ulysses4ever , I've implemented all of your changes. Not sure if the practice is for the author to mark conversations as resolved, or the one commenting, so I left all of your remarks open.

@ulysses4ever
Copy link
Collaborator

@Aspidites it's getting closer, thanks! There were three comments resolved prematurely. And indeed, it's the initial commenter's job to mark comments as resolved.

@AphonicChaos
Copy link
Contributor Author

Ooh. I know what happened. I did a force push forgetting you had suggestions that I merged. I think there one more that I forgot to reintroduce, so I'll try and find that either later this evening or tomorrow. I'll also try and see what's causing builds to fall

@AphonicChaos
Copy link
Contributor Author

Alright, @ulysses4ever, other than potentially CI, this should be good to go (reintroduced your suggestions, sorry about that).

ulysses4ever
ulysses4ever reviewed Jul 8, 2023
View reviewed changes
@ulysses4ever ulysses4ever requested review from geekosaur and ffaf1 July 8, 2023 20:33
geekosaur
geekosaur approved these changes Jul 8, 2023
View reviewed changes
Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there's a rebase / merge conflict in here with the other ticket as well, but I don't think there's anything to be done until it manifests.

@AphonicChaos
Copy link
Contributor Author

I wonder if there's a rebase / merge conflict in here with the other ticket as well, but I don't think there's anything to be done until it manifests.

I updated with the latest version of master and don't touch the files the other (if you mean @tomjaguarpaw 's) ticket touches, so there shouldn't be conflicts.

@ulysses4ever
Copy link
Collaborator

@Aspidites I think @geekosaur meant another patch: #9096 so, if you could kick out bootstrap/README, that would help me down the road.

@geekosaur
Copy link
Collaborator

Yes, that one.

@ulysses4ever ulysses4ever added the merge me Tell Mergify Bot to merge label Jul 9, 2023
@AphonicChaos
Copy link
Contributor Author

Ah, I understand now. I've kicked bootstrap/README.md out.

ffaf1
ffaf1 approved these changes Jul 9, 2023
View reviewed changes
Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@ulysses4ever ulysses4ever added merge me Tell Mergify Bot to merge squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review merge me Tell Mergify Bot to merge labels Jul 9, 2023
@AphonicChaos
Copy link
Contributor Author

No rush on my side. I just wanted to check and see if there is anything else I need to do on my end, or if it's just a matter of waiting until you folks are ready to push the button?

For instance, at work we use the squash and merge functionality from GitHub, but I wanted to make sure you all didn't expect me to squash commits on my end.

Just let me know, thanks!

@ulysses4ever ulysses4ever added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jul 10, 2023
@ulysses4ever
Copy link
Collaborator

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Jul 10, 2023

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/9)
Auto-merging CONTRIBUTING.md
Auto-merging doc/nix-local-build.rst
CONFLICT (content): Merge conflict in doc/nix-local-build.rst
error: could not apply b5707bbe6... Remove v2-prefix from other documentation where appropriate
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply b5707bbe6... Remove v2-prefix from other documentation where appropriate

err-code: 20CDB

@ulysses4ever
Copy link
Collaborator

@Aspidites no worries, thank you so much for your patience and your contribution. This PR is in the pipeline as described in https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#github-pull-request-conventions and will be merged by the end of week if all goes well. If it's not the case please ping us again. Thank you!

@ulysses4ever
Copy link
Collaborator

ulysses4ever commented Jul 10, 2023 •
edited
Loading

@Aspidites actually, there's a merge conflict it seems. Do you have energy to look into it?

@ulysses4ever
Copy link
Collaborator

@mergify squash

@ulysses4ever
Remove v2-prefix from other documentation where appropriate (haskell#…
8471233 
…9090)

Remove v2-prefix from other documentation where appropriate

* Added a changelog file

* Merge branch 'master' of github.com:haskell/cabal into remove-v2-from-docs

* Implemented PR feedback

* Reverted changes to regression tests

* Update CONTRIBUTING.md

Co-authored-by: Artem Pelenitsyn <[email protected]>

* Update bootstrap/bootstrap.py

Co-authored-by: Artem Pelenitsyn <[email protected]>

* Apply suggestions from code review

Co-authored-by: Artem Pelenitsyn <[email protected]>

* Merge branch 'master' into remove-v2-from-docs

* Removed changelog entry

* Merge branch 'remove-v2-from-docs' of github.com:aspidites/cabal into remove-v2-from-docs

* Reverted bootstrap/README.md

* Merge branch 'master' into remove-v2-from-docs
@mergify
Copy link
Contributor

mergify bot commented Jul 10, 2023

squash

✅ Pull request squashed successfully

@ulysses4ever ulysses4ever force-pushed the remove-v2-from-docs branch from fbacca7 to 8471233 Compare July 10, 2023 23:48
@ulysses4ever
Copy link
Collaborator

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Jul 10, 2023

rebase

✅ Nothing to do for rebase action

@ulysses4ever
Copy link
Collaborator

@Aspidites actually, should be good now

@geekosaur geekosaur mentioned this pull request Jul 11, 2023
Open
@mergify mergify bot merged commit bd7197b into haskell:master Jul 11, 2023
@ulysses4ever
Copy link
Collaborator

Terrific! 🎉 🎉 🎉

@AphonicChaos
Copy link
Contributor Author

Thanks, @ulysses4ever . Work has been busy so I didn't see your message about conflicts until now. Thanks for fixing it for me.

@ulysses4ever
Copy link
Collaborator

@Aspidites no problem at all! Thank you so much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ulysses4ever ulysses4ever ulysses4ever approved these changes

@geekosaur geekosaur geekosaur approved these changes

@ffaf1 ffaf1 ffaf1 approved these changes

Assignees
No one assigned
Labels
documentation merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days meta: easy re: v1-vs-v2 squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AphonicChaos @ulysses4ever @geekosaur @ffaf1