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

Fix missing manual cross-references to guide-build #159

Merged
merged 7 commits into from
Feb 23, 2023

Conversation

rptb1
Copy link
Member

@rptb1 rptb1 commented Feb 16, 2023

Fixes the Sphinx markup so that references to "Building the MPS" from the manual link correctly.

Corrects the manual Makefile that prevented the above being detected.

Adds a note to prevent a similar mistake being introduced again.

Fixes #158 .

@rptb1 rptb1 added documentation build Problems with builds and build automation labels Feb 16, 2023
@rptb1 rptb1 linked an issue Feb 19, 2023 that may be closed by this pull request
@rptb1 rptb1 self-assigned this Feb 19, 2023
@rptb1 rptb1 marked this pull request as ready for review February 19, 2023 12:10
@rptb1 rptb1 added the low risk This work is or would be of low risk of introducing defects. label Feb 20, 2023
@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

Executing proc.review.entry

  1. Start time 06:06.
  2. proc.review.entry.express: Change is small and low risk but nobody is available for express review. Plan to batch with other similar changes on 2023-02-23.
  3. Entry passed.
  4. Entry took 3 minutes.

@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

Executing proc.review.plan

  1. Start time 06:10.
  2. proc.review.plan.time: Checking should take no longer than 10 minutes.
  3. proc.review.plan.schedule: @thejayps @UNAA008 @rptb1 will review on 2023-02-12 11:00.
  4. proc.review.plan.rule: rule.generic rule.style
  5. We'll assign roles at kickoff.
  6. Planning took 6 mins.

@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

The definition in the manual here

.. c:function:: mps_res_t mps_pool_walk(mps_pool_t pool, mps_area_scan_t scan_area, void *closure)
is inconsistent with the one in the design here
``void mps_pool_walk(mps_arena_t arena, mps_pool_t pool, mps_objects_step_t step, void *p)``
producing a warning from Sphinx.

What's mysterious is why. The definition in the design document was removed in #34 which was supposedly merged.

So why is it still there in master?

@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

So why is it still there in master?

It was reintroduced in 03f5b43 ("Update the MPS Manual for compatibility with Sphinx 3.") by @gareth-rees and can be seen in this diff. Found by git bisect.

Nope, I take it back. git bisect claims this is the first bad commit, but in fact it does not contain the change. Something very odd has happened.

@rptb1
Copy link
Member Author

rptb1 commented Feb 22, 2023

Something very odd has happened.

It looks like the definitions were reintroduced during the merge of #67 , here 081d79e#diff-ad8f557930c8c718ee1919da18e146370def7ef151d51a78b164aa9a8ea066a8 . The question now is how that happened, and how to prevent it.

@rptb1
Copy link
Member Author

rptb1 commented Feb 23, 2023

Executing proc.review.kickoff

  1. Start time 13:14.
  2. @UNAA008 proc.review.role.check.{clarity,correctness,source} @rptb1 proc.review.roll.check.{backwards,consistency,convention}
  3. Logging will be at 13:45.
  4. Kickoff took 6 minutes.

Copy link
Member Author

@rptb1 rptb1 left a comment

Choose a reason for hiding this comment

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

Executing proc.review.check

  1. Start time 13:28
  2. Checked Sphinx documentation against extension again.
  3. Clean build of manual seems good.
  4. IM: There's nothing causing updates of "Old design". See also AMC pool class design document is messy #128 .
  5. Im: The design section of the manual just starts, without an intro or explanation.
  6. Im: The manual doesn't index tags.
  7. q: Are checkers looking at proc.review.check?
  8. Found: 1 major, 2 minor, 1 question.
  9. Checking took 17 mins.

Copy link
Contributor

@UNAA008 UNAA008 left a comment

Choose a reason for hiding this comment

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

I am reviewing the files for clarity and correctness and using #158 as a source document.
Q I noticed several instances of ??? in design/object-debug.txt - do these represent TODOs?
m Build.RST contains no header comments.

Comment that it takes longer to properly review the changed douments than is appropriate for this specific case.

@rptb1
Copy link
Member Author

rptb1 commented Feb 23, 2023

Executing proc.review.log

  1. Start time 13:50.
  2. Re-writing log because GitHub threw it away (again).
  3. Logging took 15 mins.
  4. Brainstorm deferred due to time constraints.

Q I noticed several instances of ??? in design/object-debug.txt - do these represent TODOs?

These are minor defects in design.mps.object-debug and need correcting.

m Build.RST contains no header comments

That's true of all the manual sources and should be fixed.

7. q: Are checkers looking at proc.review.check?

Update proc.review.ko to remind people do open and use this.

@rptb1
Copy link
Member Author

rptb1 commented Feb 23, 2023

Executing proc.review.edit

  1. Start time 15:30.

I am reviewing the files for clarity and correctness and using #158 as a source document. Q I noticed several instances of ??? in design/object-debug.txt - do these represent TODOs?

Fixed: Annotated with actual TODOs in 507ab89 .

m Build.RST contains no header comments.

Raise: #172 (comment)

Comment that it takes longer to properly review the changed douments than is appropriate for this specific case.

Answer: Noted. But please clarify "appropriate" with reference to effort per review as a whole.

IM: There's nothing causing updates of "Old design". See also #128 .

Raise: #128 (comment)

  1. Edit session suspended 15:50.
  2. Edit session resumed 17:08.

5. Im: The design section of the manual just starts, without an intro or explanation.

Fix: Added intro in 0c0c5d5 .

6. Im: The manual doesn't index tags.

Raise: #173

7. q: Are checkers looking at proc.review.check?

Fix: Added reminder to do so in 39fbd18

  1. Editing finished 17:34.
  2. Editing took 46 min.

@rptb1 rptb1 merged commit 142999b into master Feb 23, 2023
@rptb1
Copy link
Member Author

rptb1 commented Feb 23, 2023

Executing proc.merge.pull-request

  1. Start time 17:39.
  2. An automated test case for this particular issue isn't really feasible but this branch does enable warnings as errors in Sphinx and so similar problems will be detected.
  3. As I write, a few CI tests are still running, but they are on the MPS code, which this does not touch.
  4. I tested that the merged manual built cleanly, just for good measure: make -C manual clean html
  5. Merging took 9 minutes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Problems with builds and build automation documentation low risk This work is or would be of low risk of introducing defects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The manual has missing cross-references
2 participants