Skip to content

Conversation

@mortenpi
Copy link
Member

No description provided.

@mortenpi mortenpi added Type: Maintenance Skip Changelog Allows the CHANGELOG.md check to pass without edit to the file. labels May 16, 2025
Copy link
Collaborator

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Some unqualified remarks from the sidelines, please feel free to ignore. Of course overall this seems like a good idea

# the current Documenter, acting as a regression test.

# Terminate the script early if we see any non-zero error codes
set -e
Copy link
Collaborator

Choose a reason for hiding this comment

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

You do you of course, but if I may add my two cents: while I also tend to automatically write such things in shell scripts, these days I try to ask myself "why not write this in Julia" ;-). So many edge cases vanish...

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I think this is a very fair point, and I also generally tend follow that philosophy. I think what just happened here was that at first I was like "this should be like 3 shell commands", but then the complexity silently crept in 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hehe I have been in exactly that situation soooo many times ;-)

set -e

# Location if this bash script
SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
Copy link
Collaborator

Choose a reason for hiding this comment

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

trivial in julia... :-)

# Terminate the script early if we see any non-zero error codes
set -e

# Location if this bash script
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Location if this bash script
# Location of this bash script

SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
DOCUMENTER_SRC=$(dirname $(dirname "$SCRIPT_DIR"))

# Figure out the Julia binary we use. This can be passed on the command line, but
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be trivial if this script was just run in Julia?

Comment on lines +24 to +28
TMP=$(mktemp -d)
cd "$TMP"
echo "Running in: $PWD"
# .. which we also want to clean up when the script exits
trap "rm -r $TMP" EXIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

  mktempdir() do tmpdir
    cd(tmpdir)
    run(`git clone ...`)
  end

${{ runner.os }}-test-
${{ runner.os }}-
- name: "Regressions test"
run: ./.github/scripts/julia-manual-regression-test.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
run: ./.github/scripts/julia-manual-regression-test.sh
run: julia ./.github/scripts/julia-manual-regression-test.jl

Comment on lines +107 to +116
- uses: actions/cache@v4
env:
cache-name: cache-artifacts
with:
path: ~/.julia/artifacts
key: ${{ runner.os }}-test-${{ env.cache-name }}-${{ hashFiles('**/Project.toml') }}
restore-keys: |
${{ runner.os }}-test-${{ env.cache-name }}-
${{ runner.os }}-test-
${{ runner.os }}-
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't this just use julia-actions/cache ?

Suggested change
- uses: actions/cache@v4
env:
cache-name: cache-artifacts
with:
path: ~/.julia/artifacts
key: ${{ runner.os }}-test-${{ env.cache-name }}-${{ hashFiles('**/Project.toml') }}
restore-keys: |
${{ runner.os }}-test-${{ env.cache-name }}-
${{ runner.os }}-test-
${{ runner.os }}-
- uses: julia-actions/cache@v2

Copy link
Member Author

Choose a reason for hiding this comment

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

This makes sense. I think we should update the other places in the regression tests workflow too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It has been updated everywhere else by now

${{ runner.os }}-test-${{ env.cache-name }}-
${{ runner.os }}-test-
${{ runner.os }}-
- name: "Regressions test"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another request: upload the result as an artifact. That would be super useful for actually testing the result.

@fingolfin
Copy link
Collaborator

I think this would be really useful to have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Skip Changelog Allows the CHANGELOG.md check to pass without edit to the file. Type: Maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants