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 regex to remove spurious diffs in doc preview #38213

Merged
merged 4 commits into from
Jun 22, 2024

Conversation

kwankyu
Copy link
Collaborator

@kwankyu kwankyu commented Jun 13, 2024

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
Copy link

github-actions bot commented Jun 13, 2024

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
Copy link
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
Collaborator Author

kwankyu commented Jun 13, 2024

Thanks!

@kwankyu
Copy link
Collaborator Author

kwankyu commented Jun 14, 2024

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
Contributor

mkoeppe commented Jun 14, 2024

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

@mkoeppe
Copy link
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
Collaborator Author

kwankyu commented Jun 14, 2024

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
Collaborator Author

kwankyu commented Jun 14, 2024

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

@kwankyu
Copy link
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
    
<!-- ^ 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
    
<!-- ^ 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
25 of 26 checks passed
vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 4, 2024
    
<!-- ^ 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):
@mkoeppe mkoeppe added this to the sage-10.4 milestone Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants