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

Rework material on library components in Cabal Users Guide #9370

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

mpilgrem
Copy link
Collaborator

The existing material in the Cabal Users Guide on library components is written up as incremental 'developments' on top of the 'history'. This reworking aims to recast the existing material to describe the current state of affairs.

It also aims to eliminate duplication and be consistent in the terminology used, as follows: a library component can be the 'main' unnamed library or a named library (which can be referred to as a 'sublibrary'). A sublibrary can be 'public' or 'private'. A private sublibrary can be referred to as an 'internal' library.

It moves pkg-field:: visibility: visibility specifiers to the top of the discussion of library section fields, as whether a library component is 'public' or 'private' is a fundamental characteristic of the component.

It also clears the existing TODO inline the blog post. I consider that all of the substance in the blog post is now to be found in the Users Guide.

@ffaf1
Copy link
Collaborator

ffaf1 commented Oct 25, 2023

@mpilgrem I think this and #9371 overlap. Do you want me to close mine or do you prefer to rebase once mine is merged?

@ffaf1 ffaf1 added the attention: needs-help Help wanted with this issue/PR label Oct 26, 2023
@mpilgrem
Copy link
Collaborator Author

@ffaf1, I'd be happy to rebase this once yours is merged.

@ffaf1 ffaf1 removed the attention: needs-help Help wanted with this issue/PR label Oct 26, 2023
@mpilgrem
Copy link
Collaborator Author

I have rebased on #9371, but also made some further changes, namely:

  • I have explained the basis of the term 'sublibraries' in terms of 'subsidiary' or 'ancillary' libraries (about line 805)
  • I have used plainer language than the existing text at about lines 834 to 838 (explaining the significance of 'public' and 'private')
  • I have used 'expose' rather than the existing 'export' at line 919. I think modules 'export' functions etc and packages 'expose' modules.
  • I haved used plainer language than the existing text at about lines 956 and 957. In particular, I did not know what 'to vendor' meant.

@mpilgrem mpilgrem added documentation re: internal library Concerning internal libraries in packages labels Oct 27, 2023
Copy link
Collaborator

@TeofilC TeofilC left a comment

Choose a reason for hiding this comment

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

Hey, I had a read through this and had some ideas for improving this already really nice PR.
I'm not a maintainer so feel free to take my suggestions with a pinch of salt.

It might seem like a lot of comments but a lot of the suggestions are the same thing repeated a couple of times.

If you have the chance to take a look at one thing then it's having another look at the internal library example. I think some information is lost in your edits

doc/cabal-package.rst Outdated Show resolved Hide resolved
doc/cabal-package.rst Outdated Show resolved Hide resolved
doc/cabal-package.rst Outdated Show resolved Hide resolved
doc/cabal-package.rst Outdated Show resolved Hide resolved
doc/cabal-package.rst Outdated Show resolved Hide resolved
doc/cabal-package.rst Outdated Show resolved Hide resolved
doc/cabal-package.rst Outdated Show resolved Hide resolved
doc/cabal-package.rst Outdated Show resolved Hide resolved
doc/cabal-package.rst Outdated Show resolved Hide resolved
doc/cabal-package.rst Outdated Show resolved Hide resolved
@TeofilC
Copy link
Collaborator

TeofilC commented Oct 28, 2023

I haved used plainer language than the existing text at about lines 956 and 957. In particular, I did not know what 'to vendor' meant.

"to vendor" normally means to include some external source code, eg, if you are writing bindings to a C library you can vendor that library by including the C source as part of your source tarball. In the example we are vendoring parts of attoparsec by including their source in our library rather than depending on attoparsec from Hackage, eg, because it would lead to a cyclic dependency

@ulysses4ever
Copy link
Collaborator

ulysses4ever commented Oct 28, 2023

I don't think this use of the word "vendor" is universally recognized. So, I think, it should be either explained or replaced with something (macro-expanded with your explanation, perhaps).

@geekosaur
Copy link
Collaborator

It's pretty widely recognized in the developer community.

@ffaf1
Copy link
Collaborator

ffaf1 commented Oct 28, 2023 via email

@geekosaur
Copy link
Collaborator

Perhaps both (bundle (vendor)), since to me "bundle" does not imply use, just inclusion.

@mpilgrem
Copy link
Collaborator Author

mpilgrem commented Oct 28, 2023

On the use of terms such as 'to vendor', I don't know what audience the Cabal Users Guide has in mind. When I edit the Stack Users Guide, I try to have in mind somebody leaving school and about to start a degree course on computer science at university; it helps me with the 'assumed prior knowledge of the reader'. I substituted 'to incorporate' for 'to vendor'.

EDIT: I'm helped in that (in imagining my hypothetical undergraduate-to-be) by the pre-reading list of the University of Oxford: https://www.cs.ox.ac.uk/admissions/undergraduate/why_oxford/reading.html, which includes books on Haskell.

@mpilgrem
Copy link
Collaborator Author

I'll wait to see what views other people have before attempting to assimilate them all in a revised proposal.

@mpilgrem
Copy link
Collaborator Author

mpilgrem commented Oct 29, 2023

On reflection, I have force pushed an alternative now. @TeofilC's comments have encourged me to be more muscular in rephrasing existing content. I have now:

  • removed the pre-Cabal 1.8.0.4 history from the 'Package Description' introduction (about lines 11 to 13);
  • dealt with the named/unnamed v main/sub[sidiary] debate by first introducing the structure in objective terms (lines 800 to 804) and then introducing the shorthand that the guide subsequently uses consistently (lines 811 to 814);
  • included the fact that cabal-install does not, currently, support packages with sublibraries but no main library (at lines 806 to 809). EDIT: that is issue cabal-install: cannot depend on public sublibrary when the package does not contain a main library too #6038 and Cabal: cannot depend on public sublibrary when the package does not contain a main library too #8095;
  • deal with the history of the Cabal specification as regards shadowing at lines 818 to 822 and 1471 to 1476;
  • use 'Notes' to cover the history of the Cabal specification before the current specification;
  • reintroduced, parenthetically, 'vendor' and 'bundle', at line 973; and
  • been consistent with the form of the examples provided at lines 1463 to 1469.

@mpilgrem
Copy link
Collaborator Author

mpilgrem commented Nov 6, 2023

Rebased on master.

@andreabedini
Copy link
Collaborator

You all are doing an amazing job here! ❤️

@mpilgrem
Copy link
Collaborator Author

Rebased on master.

@Kleidukos Kleidukos requested a review from TeofilC December 10, 2023 00:36
@TeofilC
Copy link
Collaborator

TeofilC commented Dec 10, 2023

Thanks for the ping! I'll take another look either Tuesday or Wednesday

Copy link
Collaborator

@TeofilC TeofilC left a comment

Choose a reason for hiding this comment

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

I read over the latest version and it looks great to me! Thanks for all your work on this!

@ulysses4ever
Copy link
Collaborator

I elevated permissions for @TeofilC to Write so that his review counts towards the threshold.

@mpilgrem mpilgrem added the merge me Tell Mergify Bot to merge label Dec 13, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Dec 15, 2023
@mergify mergify bot merged commit 7478066 into master Dec 15, 2023
12 checks passed
@mergify mergify bot deleted the library-docs branch December 15, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 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: internal library Concerning internal libraries in packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants