-
Notifications
You must be signed in to change notification settings - Fork 14
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
Doctest overhaul #144
Doctest overhaul #144
Conversation
@fredrikekre @mortenpi I would like to have the docs we build on this repository be uploaded to a different place than the main docs that get built on |
I suppose those docs could serve as previews, right? We can make a new repo, staging-docs.julialang.org or something for that then. |
Sorry, I haven't really kept myself in the loop with the CI changes.. what's going on here? What builds will these be? I guess it's related to JuliaCI/julia-buildbot#273? I see that we're duplicating the docs manifest here, which doesn't seem ideal. You mentioned |
|
I'm trying to separate the "building" of docs (which is quite Julia and Documenter version-specific) with the "uploading" of docs (which should not be julia or Documenter specific, I hope!) Additionally, I am trying to minimize attack surface. The upload step has access to our DOCUMENTER_KEY. I'd like to ensure that someone doesn't do something nasty like insert a new dependency into the documenter manifest in a PR, run that PR, then get access to our DOCUMENTER_KEY. The way we do that is by signing the manifest and project TOML files, but that's a bit of a bother to maintain if we need to bump Documenter for building the docs so often. So this project is specifically only for uploading, since that should be fine to use an older Documenter version. In fact, I'd be fine with not using documenter at all if the upload step is easily recreated without importing the entire package. |
Oh I see. I'm 100% on board with that. Actually, yes, we shouldn't need any manifest at all for the (signed) upload step. For the (signed) upload step, we should download the built docs from a previous (unsigned) docbuild step, and then just do a |
ee0621c
to
fa6cbd7
Compare
Yeah, I just don't really know what this line does |
Basically |
We would just need to find a way to do that postprocessing during the (unsigned) docbuild step. |
Could you rename the |
I also agree with the goal, I think that makes a lot of sense. So this would deploy everything, i.e. Replicating |
Correct, and correct.
Could we "deploy" the docs to a local folder, then do only the |
Yeah, this is what we need. |
It shouldn't be too hard to modify Documenter such that it wouldn't |
Can't we run the full post-processing step in the unsigned builder, then all the signed builder does is copy some files somewhere? I wouldn't copy a git repository in its entirety, I would tarball up the content from the unsigned builder, then on the signed builder i would clone the relevant git repository, delete all contents within the folder, then unpack the tarball copied from the unsigned builder, and push it up. |
The postprocessing modifies the whole Pages branch (updates the symlinks, and needs to know what is on the branch to generate the version selector). I suppose we could tarball the whole working directory up and then unpack that in the signed step. Another potential option would be to generate a |
I'm happy with either (I'd probably go with the How can I get Documenter to do all that work but not push it at the end? |
So, I think we could patch Documenter like this: JuliaDocs/Documenter.jl@bd1e344, and set It seems to work fine locally, but could we test it here with the |
3b7ea62
to
0375f04
Compare
pipelines/main/misc/doctest.yml
Outdated
echo "--- Run Julia doctests" | ||
JULIA_NUM_THREADS=1 make -C doc doctest=true | ||
echo "--- Run Julia doctests/build HTML docs" | ||
make -C doc html doctest=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make -C doc html doctest=true | |
make -C doc deploy doctest=true |
I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to have DOCUMENTER_KEY
set because of https://github.com/JuliaLang/julia/blob/b11ccae719c5826cc5c1629356c0b057e25b1b45/doc/make.jl#L340. I am not sure if e.g. an empty string would work though..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, if DOCUMENTER_KEY
is set to an empty string, Documenter should be able to unpack it fine into an empty file. However, I am not sure what ssh
will do if it tries to read an empty key file (with this .ssh/config
).
So if that doesn't work, what might work is patching make.jl
further with
struct BuildBotConfig <: Documenter.DeployConfig end
# Add these two lines:
Documenter.authentication_method(::Documenter.DeployConfig) = Documenter.HTTPS
Documenter.authenticated_repo_url(::Documenter.DeployConfig) = "git@github.com:JuliaLang/docs.julialang.org.git"
to use the HTTPS
authentication path.
31d18aa
to
d27fa48
Compare
26b7b89
to
e4749ea
Compare
Alright, it's all looking good to me! I think I'm confident enough to push this through now. Will clean this up, and merge the changes to JuliaLang/julia. Thanks guys! |
@mortenpi can you merge your Documenter branch first? I figure it's best if we don't use a branch on base Julia |
Yep, will have it in 0.27.20 ASAP. |
@staticfloat you have a branch with the necessary changes to the Julia master branch, right? |
758a2ff
to
15848df
Compare
Alright, this is all looking really good! Final question: is there some way we can "deploy" these changes somewhere that's not the actual deployment target? Should we push to a non- |
We don't have anything set up for that, but pushing to a different docs.julialang.org branch should be straightforward. But why do you want to deploy those builds? |
So that people can see the changes they just made, live somewhere. |
e9edda0
to
810505b
Compare
Hmm. Would that be for PRs only then? And only for PRs from JuliaLang/julia or also forks? In principle, we have the PR previews feature, so we could push to the same |
Why would that only be safe for non-fork PRs? (Also, I’m going to merge this as-is, and we’ll continue discussion of improvements regardless) |
b5b0344
to
685149c
Compare
The way it's set up, the If we set up another repo or S3 for forks, then we could mitigate this though. For what its worth, with GHA, I have recently been leaning towards using build artifacts for previews, rather than trying to deploy them somewhere custom. Not sure if Buildkite has anything like that, or would that require setting up a custom S3? |
f65add8
to
73a8134
Compare
I want to minimize the number of times we actually build Julia on CI. This PR re-uses the output of `build_x86_64-linux-gnu`, and also adds a second, signed step to deploy the docs using an encrypted `DOCUMENTER_KEY`. Use `aws_uploader`
73a8134
to
2658a8e
Compare
I want to minimize the number of times we actually build Julia on CI.
This PR re-uses the output of
build_x86_64-linux-gnu
, and also adds asecond, signed step to deploy the docs using an encrypted
DOCUMENTER_KEY
.