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

README: document package localisation #4377

Merged
merged 7 commits into from
Aug 24, 2022
Merged

Conversation

deepfire
Copy link
Contributor

@deepfire deepfire commented Aug 23, 2022

In the README:

  1. Add a ToC.
  2. Fix the structure, so the ToC actually makes sense.
  3. Fix some minor RST syntax issues.
  4. Some very minor fixes.
  5. Expand the Development section with the workbench usage info.
  6. Add cabal-plan and graphmod to the workbench shell.
  7. Makes the node start on Darwin, when metric collection is enabled, by fixing workbench | macOS: node fails with divide-by-zero #4380

@deepfire deepfire force-pushed the document-package-localisation branch 8 times, most recently from d995f59 to dff6fe7 Compare August 23, 2022 11:41
@deepfire deepfire force-pushed the document-package-localisation branch 7 times, most recently from d3aaa3a to c737bcf Compare August 23, 2022 12:06
@deepfire deepfire force-pushed the document-package-localisation branch 11 times, most recently from 9d08d83 to 87becaf Compare August 23, 2022 12:50
@deepfire deepfire force-pushed the document-package-localisation branch 7 times, most recently from 2614759 to bc39a1b Compare August 23, 2022 16:53
@deepfire deepfire force-pushed the document-package-localisation branch 2 times, most recently from 85d8d49 to 6fcd6dd Compare August 23, 2022 17:15
@newhoggy
Copy link
Contributor

I'm interested to know if the new instructions will work on a Mac (Intel or M1). I think this should be documented to let people know whether the instructions are worth trying on these platforms.

@deepfire
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 24, 2022
4377: README:  document package localisation r=deepfire a=deepfire

In the README:

1. Add a ToC.
2. Fix the structure, so the ToC actually makes sense.
3. Fix some minor RST syntax issues.
4. Some very minor fixes.
5. Expand the Development section with the workbench usage info.
6. Add `cabal-plan` and `graphmod` to the workbench shell.

Co-authored-by: Kosyrev Serge <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 24, 2022

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"Waiting on code owner review from fmaste and/or mgmeier.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

README.rst Outdated
Normally, to work on ``cardano-node`` dependencies in the context of the node itself, one needs to go through an expensive multi-step process, mandated by the Nix model:
- Make changes to a dependency (say ``ouroboros-consensus-cardano`` in the ``ouroboros-network`` repository), commit them & push to Github.
- Calculate & bump the pin for that dependency in ``cardano-node``, inside its ``cabal.project``.
- Re-enter the Nix shell in ``cardano-node``, suffering full rebuilds of everything.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of describing this workflow if the actual thing you want to document is below? Also, the language used here is sarcastic and makes the reader question why we are using nix if it is resulting in "suffering full rebuilds of everything".

I would suggest to frame the whole section of "Dependency localisation" in a more positive way.

Copy link
Contributor Author

@deepfire deepfire Aug 24, 2022

Choose a reason for hiding this comment

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

Also, the language used here is sarcastic and makes the reader question why we are using nix if it is resulting in "suffering full rebuilds of everything".

Was not intended to be sarcastic at all -- just recognising a difficult reality we all had to deal with (and that's for a reason, also).

But perhaps you are right -- I should find a more neutral wording.

@deepfire deepfire force-pushed the document-package-localisation branch from 6fcd6dd to 9b5fe60 Compare August 24, 2022 07:01
@mgmeier
Copy link
Contributor

mgmeier commented Aug 24, 2022

I'm interested to know if the new instructions will work on a Mac (Intel or M1). I think this should be documented to let people know whether the instructions are worth trying on these platforms.

I've tested both cabal-plan and graphmod build inputs for workbench successfully on an Intel Mac.

@deepfire deepfire force-pushed the document-package-localisation branch from 8e9e035 to f75b4b3 Compare August 24, 2022 10:44
@deepfire
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 24, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit e36c1a7 into master Aug 24, 2022
@iohk-bors iohk-bors bot deleted the document-package-localisation branch August 24, 2022 11:34
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.

5 participants