Skip to content

Fix regex to remove spurious diffs in doc preview#38213

Merged
vbraun merged 4 commits intosagemath:developfrom
kwankyu:p/fix-for-changes
Jun 22, 2024
Merged

Fix regex to remove spurious diffs in doc preview#38213
vbraun merged 4 commits intosagemath:developfrom
kwankyu:p/fix-for-changes

Conversation

@kwankyu
Copy link
Copy Markdown
Collaborator

@kwankyu kwankyu commented Jun 13, 2024 •
edited
Loading

There was an error in the regex, seen in

https://preview-38210--sagemath.netlify.app/changes

Also removed most spurious diffs: see changes of this PR.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@kwankyu kwankyu changed the title Fix regex to remove spurious diffs Fix regex to remove spurious diffs in doc preview Jun 13, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 13, 2024 •
edited
Loading

Documentation preview for this PR (built with commit d1220c9; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@kwankyu kwankyu force-pushed the p/fix-for-changes branch from 21b19b7 to b932246 Compare June 13, 2024 11:15
@kwankyu kwankyu requested a review from mkoeppe June 13, 2024 13:47
mkoeppe
mkoeppe approved these changes Jun 13, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mkoeppe mkoeppe left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

@kwankyu
Copy link
Copy Markdown
Collaborator Author

kwankyu commented Jun 13, 2024

Thanks!

@kwankyu
Copy link
Copy Markdown
Collaborator Author

kwankyu commented Jun 14, 2024 •
edited
Loading

I am trying to understand where the last diff left in "changes" come from.

"changes" is the result of git diff between the "new" doc built from the branch of this PR and the "old" doc built for the latest release. Hence the "old" doc should be the doc in ghcr.io/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:dev (or 10.4.beta9).

According to this run of doc-build workflow:

https://github.com/sagemath/sage/actions/runs/9500068057/job/26182310478?pr=38213

"Build Docker Image" step builds docker image localhost:5000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci from ghcr.io/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:dev. But then In the next "Start container" step,

docker run --name BUILD -dit \
             --mount type=bind,src=$(pwd),dst=$(pwd) \
             --workdir $(pwd) \
             localhost:[5](https://github.com/sagemath/sage/actions/runs/9500068057/job/26182310478?pr=38213#step:10:5)000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci /bin/sh
  shell: /usr/bin/bash -e {0}
  env:
    TOX_ENV: docker-ubuntu-jammy-standard-incremental
    BUILD_IMAGE: localhost:5000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci
    FROM_DOCKER_REPOSITORY: ghcr.io/sagemath/sage/
    FROM_DOCKER_TARGET: with-targets
    FROM_DOCKER_TAG: dev
    EXTRA_CONFIGURE_ARGS: --enable-fat-binary
Unable to find image 'localhost:5000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci' locally
ci: Pulling from sagemath/sage/sage-ubuntu-jammy-standard-with-targets

it seems that it fails to find the image localhost:5000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci, and instead pull sagemath/sage/sage-ubuntu-jammy-standard-with-targets with no tag. Thus, I think, it pulls the image with the default latest tag. But then according to

https://github.com/sagemath/sage/pkgs/container/sage%2Fsage-ubuntu-jammy-standard-with-targets/versions

sagemath/sage/sage-ubuntu-jammy-standard-with-targets:latest would be 10.3. Hence all screws up... Am I right? This explanation seems still strange since 10.3 is too old version...

@mkoeppe
Copy link
Copy Markdown
Contributor

mkoeppe commented Jun 14, 2024

The incremental build is always starting from the tag :dev (see FROM_DOCKER_TAG)

@mkoeppe
Copy link
Copy Markdown
Contributor

mkoeppe commented Jun 14, 2024

Unable to find image 'localhost:5000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci' locally
ci: Pulling from sagemath/sage/sage-ubuntu-jammy-standard-with-targets

This message just means it's pulling what was just built.

@kwankyu
Copy link
Copy Markdown
Collaborator Author

kwankyu commented Jun 14, 2024 •
edited
Loading

Ah, I see. So localhost:5000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci is just ghcr.io/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:dev.

I looked into ghcr.io/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:dev, and indeed there are lines:

<span class="go">  --sws2rst &lt;sws doc&gt; -- Generates a reStructuredText source file from</span>
<span class="go">                         a Sage worksheet (.sws) document.</span>
<span class="go">                         (not installed currently, run sage -i sage_sws2rst)</span>

<span class="go">Valgrind memory debugging:</span>

Hence the diff is genuine.

On the other hand, looking into the source file of the page https://sagemath.netlify.app/html/en/reference/repl/options, which was uploaded from localhost:5000/sagemath/sage/sage-ubuntu-jammy-standard-with-targets:ci, I see

<span class="go">  --sws2rst &lt;sws doc&gt; -- Generates a reStructuredText source file from</span>
<span class="go">                         a Sage worksheet (.sws) document.</span>
  
<span class="go">Valgrind memory debugging:</span>

where, as you see, there is no line

<span class="go">                         (not installed currently, run sage -i sage_sws2rst)</span>

!

@kwankyu
Copy link
Copy Markdown
Collaborator Author

kwankyu commented Jun 14, 2024 •
edited
Loading

Hmm presently https://sagemath.netlify.app/html/en/reference/repl/options is doc for PR #38205! Strange things are happening...

@kwankyu
Copy link
Copy Markdown
Collaborator Author

kwankyu commented Jun 14, 2024

presently https://sagemath.netlify.app/html/en/reference/repl/options is doc for PR #38205!

This is fixed by #38220.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
c7598cf 
sagemathgh-38213: Fix regex to remove spurious diffs in doc preview
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

There was an error in the regex, seen in

https://preview-38210--sagemath.netlify.app/changes

Also removed most spurious diffs: see [changes](https://preview-38213--
sagemath.netlify.app/changes) of this PR.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38213
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Jun 16, 2024
eaef75a 
sagemathgh-38213: Fix regex to remove spurious diffs in doc preview
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

There was an error in the regex, seen in

https://preview-38210--sagemath.netlify.app/changes

Also removed most spurious diffs: see [changes](https://preview-38213--
sagemath.netlify.app/changes) of this PR.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38213
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
@vbraun vbraun merged commit 883c168 into sagemath:develop Jun 22, 2024
@kwankyu kwankyu mentioned this pull request Jun 25, 2024
Merged
5 tasks
vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 4, 2024
31b186c 
sagemathgh-38274: Fix broken anchors in CHANGES.html of doc preview
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

broken by careless code in sagemath#38213:
```
          # Restore the new doc from changes by "wipe out"
          (cd docs && git checkout -f)
```
which also wipes out anchors planted by `.ci/create-changes-html.sh`!

To reviewer: changes over sagemath#38220 is in https://github.com/sagemath/sage/
pull/38274/commits/d2d1514148f46a4ceeea650eab1fd0a0800ebbc9

Tested in :
https://github.com/kwankyu/sage/actions/runs/9659812609/job/26643993813.
Try to open https://doc-pr-43--sagemath-
test.netlify.app/html/en/reference/references/#hunk2  to check

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->

sagemath#38220
    
URL: sagemath#38274
Reported by: Kwankyu Lee
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mkoeppe mkoeppe mkoeppe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: documentation c: scripts v: minimal

Projects

None yet

Milestone

sage-10.4

Development

Successfully merging this pull request may close these issues.

3 participants

@kwankyu @mkoeppe @vbraun