Check env vars for non-Travis CI platforms #1067

asinghvi17 · 2019-07-13T09:19:15Z

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 · 2019-07-13T09:21:10Z

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

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

mortenpi · 2019-07-13T23:24:08Z

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

Co-Authored-By: Morten Piibeleht <morten.piibeleht@gmail.com>

asinghvi17 · 2019-07-14T01:17:06Z

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 · 2019-07-14T01:49:59Z

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

mortenpi · 2019-07-14T21:46:12Z

src/Writers/LaTeXWriter.jl

+            @info "No tag detected..."
+            else
+                ""
+            end


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.

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

mortenpi · 2019-07-14T21:50:49Z

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

asinghvi17 · 2019-07-15T02:01:28Z

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

CHANGELOG.md

Co-Authored-By: Morten Piibeleht <morten.piibeleht@gmail.com>

mortenpi

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

src/Writers/LaTeXWriter.jl

src/Documenter.jl

Co-Authored-By: Morten Piibeleht <morten.piibeleht@gmail.com>

asinghvi17 · 2019-07-16T03:10:02Z

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...

src/Writers/LaTeXWriter.jl

CHANGELOG.md

…ter.jl into as/generic-ci

asinghvi17 · 2019-07-23T03:47:12Z

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

…ter.jl into as/generic-ci

marekdedic · 2019-07-23T22:33:15Z

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 · 2019-07-23T22:50:41Z

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 · 2019-07-24T01:57:05Z

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 · 2019-07-24T06:19:28Z

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 · 2019-07-24T06:23:30Z

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.

asinghvi17 · 2019-07-24T07:49:09Z

src/Writers/LaTeXWriter.jl

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

 """
 module LaTeXWriter
-import ...Documenter: Documenter
+import ...Documenter: Documenter, read_ci_env


WARNING: could not import Documenter.read_ci_env into LaTeXWriter

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 · 2019-07-28T23:45:51Z

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.

mortenpi

Still a few things to work out here.

mortenpi · 2019-07-29T02:45:59Z

src/Writers/LaTeXWriter.jl

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


Remove the empty line:

Suggested change

mortenpi · 2019-07-29T02:49:38Z

src/Documenter.jl

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



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

mortenpi · 2019-07-29T02:51:03Z

src/Documenter.jl

+                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.
+               """


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?

mortenpi · 2019-07-29T02:51:18Z

src/Documenter.jl

+    end
+
+    # Get authentication key.
+    documenter_key = get(ENV, "DOCUMENTER_KEY",       "")


Suggested change

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

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

mortenpi · 2019-07-29T02:52:54Z

src/Documenter.jl

-    ## 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)


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

mortenpi · 2019-07-29T02:53:51Z

src/Writers/LaTeXWriter.jl

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

 """
 module LaTeXWriter
-import ...Documenter: Documenter
+import ...Documenter: Documenter, read_ci_env


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 · 2019-07-29T02:58:31Z

src/CISystems.jl

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


It's not actually returning anything?

mortenpi · 2019-07-29T02:58:54Z

src/CISystems.jl

+"""
+function read_ci_env(returnfinaldeploy=false; uploader::CI_SYSTEM = TRAVIS)
+
+    arraylength = returnfinaldeploy ? 6 : 5


mortenpi · 2019-07-29T03:01:43Z

src/CISystems.jl

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


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.

mortenpi · 2019-07-29T03:02:46Z

src/Writers/LaTeXWriter.jl

@@ -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] : ""


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 · 2019-08-05T14:55:39Z

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 · 2019-08-06T02:01:44Z

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.

simonbyrne · 2019-08-14T19:58:21Z

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

asinghvi17 · 2019-08-14T22:26:34Z

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 · 2023-11-01T22:03:37Z

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

asinghvi17 added 2 commits January 1, 2019 05:39

First pass at decoupling manual deploy from generic CI

42af534

asinghvi17 mentioned this pull request Jul 13, 2019

Towards a new version JuliaPlots/MakieGallery.jl#74

Merged

implement preferred uploader

7467920

mortenpi added the Type: Enhancement label Jul 13, 2019

mortenpi added this to the 0.23.0 milestone Jul 13, 2019

mortenpi approved these changes Jul 13, 2019

View reviewed changes

src/Documenter.jl Outdated Show resolved Hide resolved

fredrikekre reviewed Jul 14, 2019

View reviewed changes

src/Documenter.jl Outdated Show resolved Hide resolved

Better pull request check message

2986dab

Co-Authored-By: Morten Piibeleht <morten.piibeleht@gmail.com>

asinghvi17 added 3 commits July 14, 2019 07:18

Make LaTeXWriter check for tag on all CIs

b970a99

Create and export a CI_SYSTEM enum

e48fafa

Document and add CHANGELOG

3bef32b

asinghvi17 and others added 3 commits July 14, 2019 14:10

fix overlapping variable names

2175bb1

Merge branch 'master' into as/generic-ci

b3b409b

Add PR nr, don't have it experimental

06422a8

mortenpi reviewed Jul 14, 2019

View reviewed changes

mortenpi reviewed Jul 15, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

asinghvi17 and others added 3 commits July 15, 2019 07:42

Remove exp badge, add untested info

b4c7620

Co-Authored-By: Morten Piibeleht <morten.piibeleht@gmail.com>

read CI env in fcn, better manual deploy

1ef6c5c

FIX TYPE

17acc66

mortenpi reviewed Jul 15, 2019

View reviewed changes

asinghvi17 and others added 2 commits July 16, 2019 07:14

Apply suggestions from review

0dea970

Co-Authored-By: Morten Piibeleht <morten.piibeleht@gmail.com>

Add functions to docs (wow that was easy)

26c5aff

mortenpi added this to the 0.24.0 milestone Jul 17, 2019

asinghvi17 commented Jul 23, 2019

View reviewed changes

src/Writers/LaTeXWriter.jl Outdated Show resolved Hide resolved

Change to namedtuple return type

588fc01

asinghvi17 commented Jul 23, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Remove manual tag setting from changelog

e550502

asinghvi17 commented Jul 23, 2019

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

asinghvi17 added 2 commits July 23, 2019 09:15

Remove empty line in CHANGELOG.md

844d1f4

Merge branch 'as/generic-ci' of https://github.com/asinghvi17/Documen…

61ecc09

…ter.jl into as/generic-ci

asinghvi17 added 3 commits July 23, 2019 09:18

Merge branch 'master' into as/generic-ci

7a256b8

Import read_ci_env to LaTeXWriter

51eb94f

Merge branch 'as/generic-ci' of https://github.com/asinghvi17/Documen…

9d3f16e

…ter.jl into as/generic-ci

asinghvi17 commented Jul 24, 2019

View reviewed changes

asinghvi17 added 2 commits July 25, 2019 13:23

Change structure so that LaTeXWriter can read CI variables

338d05d

fix small bug

d623f78

mortenpi reviewed Jul 29, 2019

View reviewed changes

mortenpi mentioned this pull request Aug 8, 2019

New API for deployment #1087

Closed

mortenpi removed this from the 0.24.0 milestone Dec 13, 2019

mortenpi closed this Nov 1, 2023

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

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

Check env vars for non-Travis CI platforms #1067

Check env vars for non-Travis CI platforms #1067

Conversation

asinghvi17 commented Jul 13, 2019

asinghvi17 commented Jul 13, 2019

mortenpi left a comment

Choose a reason for hiding this comment

mortenpi commented Jul 13, 2019

asinghvi17 commented Jul 14, 2019

asinghvi17 commented Jul 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortenpi commented Jul 14, 2019

asinghvi17 commented Jul 15, 2019

mortenpi left a comment

Choose a reason for hiding this comment

asinghvi17 commented Jul 16, 2019

asinghvi17 commented Jul 23, 2019

marekdedic commented Jul 23, 2019

marekdedic commented Jul 23, 2019

asinghvi17 commented Jul 24, 2019

marekdedic commented Jul 24, 2019

marekdedic commented Jul 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortenpi commented Jul 28, 2019

mortenpi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekdedic commented Aug 5, 2019

mortenpi commented Aug 6, 2019

simonbyrne commented Aug 14, 2019

asinghvi17 commented Aug 14, 2019

mortenpi commented Nov 1, 2023