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

Check env vars for non-Travis CI platforms #1067

Closed
wants to merge 29 commits into from

Conversation

asinghvi17
Copy link
Contributor

Special cases are now Travis, Gitlab, Drone, Cirrus and Appveyor.

Not all of these CI services provide the same information as Travis
does, or in the same way, so some massaging of variable contents is
required. Appveyor is arguably one of the worse ones here.

This is untested on most of these CI systems at the time this was
committed; hence, any feedback is appreciated.

Special cases are now Travis, Gitlab, Drone, Cirrus and Appveyor. 

Not all of these CI services provide the same information as Travis 
does, or in the same way, so some massaging of variable contents is 
required.  Appveyor is arguably one of the worse ones here.

This is untested on most of these CI systems at the time this was 
committed; hence, any feedback is appreciated.
@asinghvi17
Copy link
Contributor Author

We will need some way to check which CI service is the "preferred uploader of documentation". Perhaps we could introduce a keyword argument to deploydocs? uploader = "TRAVIS", could be any of the CI environment variable names.

@mortenpi mortenpi added this to the 0.23.0 milestone Jul 13, 2019
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

This is awesome! Can be merged as is as far as I can tell. Would you mind adding an entry to the CHANGELOG?

src/Documenter.jl Outdated Show resolved Hide resolved
@mortenpi
Copy link
Member

Goes a long way towards fixing #988. We're only missing BitBucket now I think.

Regarding GitLab -- the assumption is that you'll still be pushing the pages to GitHub though, since GitLab pages uses a different system?

src/Documenter.jl Outdated Show resolved Hide resolved
Co-Authored-By: Morten Piibeleht <morten.piibeleht@gmail.com>
@asinghvi17
Copy link
Contributor Author

Regarding GitLab -- the assumption is that you'll still be pushing the pages to GitHub though, since GitLab pages uses a different system?

Yes, for now - I'll add a statement in the docs about that. It seems that with Gitlab CI you only need to push to a different folder, though, so that may not be too hard. They claim you can use any static site generator to create the website.

@asinghvi17
Copy link
Contributor Author

Thanks for the review, @mortenpi and @fredrikekre!
I think this is now good to go from my side.

@info "No tag detected..."
else
""
end
Copy link
Member

Choose a reason for hiding this comment

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

The indentation is a bit broken here.

But in any case -- this actually duplicates the code above. Could we refactor the if into a function that we can re-use here and above?

I am also wondering if it might be better to make the @infos @debugs. Although, there might be value in being verbose about this automagic behavior here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make it a function, will take a while though - dont have too much free time today, so maybe in the evening.

@mortenpi
Copy link
Member

Was there are a reason, by the way, why you wanted to mark this "experimental"?

@asinghvi17
Copy link
Contributor Author

Just because I haven't tested on all of these systems.

CHANGELOG.md Outdated Show resolved Hide resolved
asinghvi17 and others added 3 commits July 15, 2019 07:42
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Having the ability to easily deploy from manually is also a great thing to have. It might be worth iterating on it a bit though. One option is to do that in a separate PR, as not to hold up the CI service support stuff.

The documentation CI builds complains about missing docstrings in the manual. Could we put:

  • CI_SYSTEM in lib/public.md
  • read_ci_env in lib/internals/documenter.md

src/Documenter.jl Outdated Show resolved Hide resolved
src/Documenter.jl Outdated Show resolved Hide resolved
src/Documenter.jl Show resolved Hide resolved
src/Documenter.jl Outdated Show resolved Hide resolved
src/Writers/LaTeXWriter.jl Outdated Show resolved Hide resolved
src/Writers/LaTeXWriter.jl Outdated Show resolved Hide resolved
src/Documenter.jl Outdated Show resolved Hide resolved
src/Documenter.jl Outdated Show resolved Hide resolved
src/Documenter.jl Show resolved Hide resolved
src/Documenter.jl Outdated Show resolved Hide resolved
asinghvi17 and others added 2 commits July 16, 2019 07:14
Co-Authored-By: Morten Piibeleht <morten.piibeleht@gmail.com>
@asinghvi17
Copy link
Contributor Author

Thanks @mortenpi for the extensive review! I've added comments in the reviews.

I can separate the local dev changes into another PR (if this one is OK with tests and all), but it will take me some time - I'm sending my laptop out for repairs, and would need to set up the loaner...

@mortenpi mortenpi added this to the 0.24.0 milestone Jul 17, 2019
CHANGELOG.md Outdated Show resolved Hide resolved
@asinghvi17
Copy link
Contributor Author

@mortenpi , sorry for the delay on this. I've removed the manual deployment stuff (I think) and this should be final if tests work.

@marekdedic
Copy link

Hi,
joining a bit late I suppose, but I wanted to make Documenter work on Circle CI and found this...

I see you are doing a lot of mingling of the environment variables - is that really necessary? Wouldn't a better solution be to leave it to the CI config, which already does a lot of that (although I am not sure it does everything)

For example, instead of checking TRAVIS_PULL_REQUEST == false, I think it would be better to just put this in your config:

jobs:
  include:
    - stage: "Documentation"
      if: type != pull_request
      julia: 1.0
      os: linux
      script:
        - julia --project=docs/ -e 'using Pkg; Pkg.develop(PackageSpec(path=pwd()));
                                               Pkg.instantiate()'
        - julia --project=docs/ docs/make.jl
      after_success: skip

https://docs.travis-ci.com/user/conditional-builds-stages-jobs/

That way, the core of Documenter can be (maybe entirely) CI-independent. Of course, supplying example configurations for some of the more common CIs would be great for the users.

@marekdedic
Copy link

Follow up on my previous comment - a list of conditions on Travis, corresponding to the current checks:

  • occursin(travis_repo_slug, repo) - I am not really sure why this check is there, maybe because of forks? Anyway, can be replaced by if: repo = owner/name
  • travis_pull_request == "false" can be replaced by if: type != pull_request
  • isempty(travis_tag) || occursin(Base.VERSION_REGEX, travis_tag) can be replaced, but it's very ugly: if: tag = "" OR tag =~ ^v?(\d+)(?:\.(\d+))?(?:\.(\d+))?(?:(-)|([a-z][0-9a-z-]*(?:\.[0-9a-z-]+)*|-(?:[0-9a-z-]+\.)*[0-9a-z-]+)?(?:(\+)|(?:\+((?:[0-9a-z-]+\.)*[0-9a-z-]+))?))$
  • !isempty(travis_tag) || travis_branch == devbranch can be replaced if we know the value of devbranch: if: tag != "" OR branch = devbranch
  • travis_event_type != "cron" can be replaced by if: type != cron

What probably cannot be very easily replaced is the check for the existence of DOCUMENTER_KEY. On the other hand, I think the key shouldn't be passed as an environment variable in the first place - as far as I understand it, Travis can set up the key to be used by ssh: https://docs.travis-ci.com/user/private-dependencies/#user-key

I haven't used Travis in several years, so maybe I got something wrong - in such a case I would be glad to be corrected.

@asinghvi17
Copy link
Contributor Author

I think the logic here is that you want to be able to build documentation on every CI run, possibly inspect it, but not actually deploy it.

@marekdedic
Copy link

Hmm, I see what you mean...

On CircleCI I would then separate into 2 jobs the building of the docs from their deployment....

However, it seems to me now that I'm looking into the docs that in Travis, there is no easy way to share data between jobs :( So you'd need to have 2 jobs and build the docs in both and that is just bad design.

I still think the deployment should be independent from the CI you are using, but as far as I can tell Travis offers no easy way to solve the problem you're taking about.

@marekdedic
Copy link

By the way, regarding #1067 (comment) - yes, on GitLab, you don't push anything, you just tell the CI "this folder has the content of the pages". So you don't even need the deploydocs function. I can provide an example config.

@@ -13,7 +13,7 @@ navigation menu. It goes into the `\\title` LaTeX command.

"""
module LaTeXWriter
import ...Documenter: Documenter
import ...Documenter: Documenter, read_ci_env
Copy link
Contributor Author

Choose a reason for hiding this comment

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

WARNING: could not import Documenter.read_ci_env into LaTeXWriter

Copy link
Member

Choose a reason for hiding this comment

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

Did you resolve this? I didn't see this locally myself. But is/was this due to read_ci_env living in a submodule now?

@mortenpi
Copy link
Member

Sorry for the delay with reviewing this, will take another look at the code tonight.

@asinghvi17 is right in that the make.jl script should always run on CI even if no deployment occurs. E.g. to catch documentation build errors in PRs. Also, deploydocs should not deploy anything if you're running make.jl locally.

So I am not sure we want to decouple the deploy checks from deploydocs. It would also mean that there would be more logic that the user has to maintain. deploydocs is just a convenience function for the user -- you could always write your own deployment phase.

That is not to say that there aren't refactorings that couldn't be done, but I think these things go beyond this PR. Feel free to open up a separate issue though to discuss this further.

On the other hand, I think the key shouldn't be passed as an environment variable in the first place - as far as I understand it, Travis can set up the key to be used by ssh: https://docs.travis-ci.com/user/private-dependencies/#user-key

That feature is only for private repositories, no?

By the way, regarding #1067 (comment) - yes, on GitLab, you don't push anything, you just tell the CI "this folder has the content of the pages". So you don't even need the deploydocs function. I can provide an example config.

I'd say that you'd still want one, ideally. The idea of deploydocs is that the user would have to do as little as possible. I do believe you are right that on GitLab the deploydocs implementation would be much simpler.

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Still a few things to work out here.

let tag = get(ENV, "TRAVIS_TAG", "")
if occursin(Base.VERSION_REGEX, tag)
let tag = read_ci_env()[:tag]

Copy link
Member

Choose a reason for hiding this comment

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

Remove the empty line:

Suggested change

@@ -667,100 +695,106 @@ function git_push(
end
chmod(keyfile, 0o600)

Copy link
Member

Choose a reason for hiding this comment

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

Could we undo the changes below this line? They are pretty extensive, touching fragile code and not really relevant to the CI changes.

Please set the appropriate environment variables before you try to deploy.
These environment variables can be seen in the documentation for `deploydocs`,
which can be accessed by `?deploydocs` in the REPL.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. This will now print this error every time you build locally, which is a bit misleading. Could we just @info here with a one-line comment about the CI variable not being defined?

end

# Get authentication key.
documenter_key = get(ENV, "DOCUMENTER_KEY", "")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
documenter_key = get(ENV, "DOCUMENTER_KEY", "")
documenter_key = get(ENV, "DOCUMENTER_KEY", "")

## The deploydocs' repo should match TRAVIS_REPO_SLUG
repo_ok = occursin(travis_repo_slug, repo)
## The deploydocs' repo should match the repo slug provided by CI
repo_ok = occursin(repo_slug, repo)
Copy link
Member

Choose a reason for hiding this comment

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

I'm getting a ERROR: LoadError: UndefVarError: repo_slug not defined here.

@@ -13,7 +13,7 @@ navigation menu. It goes into the `\\title` LaTeX command.

"""
module LaTeXWriter
import ...Documenter: Documenter
import ...Documenter: Documenter, read_ci_env
Copy link
Member

Choose a reason for hiding this comment

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

Did you resolve this? I didn't see this locally myself. But is/was this due to read_ci_env living in a submodule now?

returnfinaldeploy && begin ret = (ret..., uploader = uploader == nothing) end

end
end
Copy link
Member

Choose a reason for hiding this comment

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

It's not actually returning anything?

"""
function read_ci_env(returnfinaldeploy=false; uploader::CI_SYSTEM = TRAVIS)

arraylength = returnfinaldeploy ? 6 : 5
Copy link
Member

Choose a reason for hiding this comment

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

Unused

Furthermore, if `returnfinaldeploy` is true, the function will check whether the
current CI provider matches `uploader` (set by default to Travis).

Returns a NamedTuple.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe, for a more cleaner API here, it would be better if read_ci_env would just return nothing if the CI env can not be detected? That way the call sites do not have to do the haskey(ENV, "CI") check, which may actually be different on different CI systems (in general). So instead, they would check if this returned nothing or not, and decide whether we're in CI based on that.

@@ -188,6 +189,8 @@ end
function writeheader(io::IO, doc::Documents.Document)
custom = joinpath(doc.user.root, doc.user.source, "assets", "custom.sty")
isfile(custom) ? cp(custom, "custom.sty"; force = true) : touch("custom.sty")

tag = haskey(ENV, "CI") ? read_ci_env(false)[:tag] : ""
Copy link
Member

Choose a reason for hiding this comment

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

You pass false here, but not above?

Not sure if the finaldeploy check should really be in the read_ci_env function like this in the first place.

@marekdedic
Copy link

Hi,
well, that's what I see as the biggest weakness of this approach - stuffing CI logic into this package and the deploydocs function. In my use case, I have make.jl for building the docs and deploy.jl for deploying them. Then I configure the CI make.jl if I just want to build the docs (and save them as an artefact for example) and both if I want to deploy the docs as well...

However, it is probably too late to change that now and force users to have two files.

I still stand behind the idea that CI logic should be left to the CI system and not to the deploy function. However, I am now not so sure how to do that properly.

Regarding the GitLab pages issue, I agree, having the deploydocs function there and doing most of the work makes sense to me there.

@mortenpi
Copy link
Member

mortenpi commented Aug 6, 2019

I still stand behind the idea that CI logic should be left to the CI system and not to the deploy function. However, I am now not so sure how to do that properly.

You are not wrong in the sense that we are currently missing a way to just easily deploy to gh-pages (#210). You need to do everything by hand if you need that. But deploydocs is just a convenience function for Travis users (but with this PR, will hopefully also work on a few other CI systems).

We definitely want some API to just do deployment (which deploydocs would internally call, most likely). But it does need some thought and refactoring. One interface that I have been contemplating that would achieve that is to pass arguments to make.jl. I.e. you'd just have to call something like julia docs/make.jl deploy to deploy the docs. See #75, this gist and this.

@mortenpi mortenpi mentioned this pull request Aug 8, 2019
@simonbyrne
Copy link
Contributor

Any movement on this? I would love to add support for Azure Pipelines as well.

@asinghvi17
Copy link
Contributor Author

I'll create a more minimized PR in a couple of days, hopefully that simplifies matters - I seem to have touched too much code here.

@mortenpi mortenpi removed this from the 0.24.0 milestone Dec 13, 2019
@mortenpi
Copy link
Member

mortenpi commented Nov 1, 2023

This is very stale, so I'll go ahead an close this. Also, I think deploydocs kinda works this way now with the DeployConfigs.

@mortenpi mortenpi closed this Nov 1, 2023
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.

5 participants