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

[ci] expose more functions on Target.v #430

Merged
merged 1 commit into from
Jan 9, 2017
Merged

Conversation

avsm
Copy link
Collaborator

@avsm avsm commented Jan 5, 2017

These are convenience functions that match the equivalents for
the unresolved Target.t, used in mirage-ci

@@ -87,6 +87,10 @@ let path = function

type v = [ `PR of PR.t | `Ref of Ref.t ]

let pp_v f = function
| `PR pr -> Fmt.pf f "pr/%a" PR.pp pr
| `Ref x -> Fmt.pf f "refs/%a" Ref.pp x
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems strange that one case is plural and the other isn't.

Looks like we use pr and ref in URLs, but prs/heads/tags when parsing. Git uses .git/refs.

Copy link
Member

Choose a reason for hiding this comment

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

yes not sure where this is used. I've tried to not use s in the datakit/github bridge, but indeed Git is putting s everywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm only using this pp_v function for branch name calculation, so the values don't seem significant (but should be consistent). I'll modify it to be plural to match Git then.

These are convenience functions that match the equivalents for
the unresolved `Target.t`, used in mirage-ci

Signed-off-by: Anil Madhavapeddy <anil@recoil.org>
@avsm
Copy link
Collaborator Author

avsm commented Jan 9, 2017

the CI seems to have failed for reasons other than this patch.

@samoht
Copy link
Member

samoht commented Jan 9, 2017

minor nitpick: can you change back to use singular for pr and ref to be more consistent with the rest of the bridge?

@talex5
Copy link
Contributor

talex5 commented Jan 9, 2017

What causes this?

[WARNING] External solver failed with inconsistent return value. Request saved
          to "/home/opam/.opam/log/solver-error-19-1.cudf"
[ERROR] External solver failure, please fix your installation and check
        /home/opam/.opam/config and variable $OPAMEXTERNALSOLVER.
        You may also retry with option --use-internal-solver

@avsm
Copy link
Collaborator Author

avsm commented Jan 9, 2017

@talex: #433 (comment)

@avsm
Copy link
Collaborator Author

avsm commented Jan 9, 2017

@samoht I think pp_v here should be consistent with pp in the same module, which uses prs and not pr.

@talex5
Copy link
Contributor

talex5 commented Jan 9, 2017

CentOS and Fedora are the only containers that use the Docker Cloud-hosted solver.ocaml.io, so you will see that error if the infra is down. The builds can be restarted.

But these are alpine images.

@samoht
Copy link
Member

samoht commented Jan 9, 2017

@avsm ha yes you are right. I'm fine with the s then :-)

@talex5 talex5 merged commit 6811870 into moby:master Jan 9, 2017
@avsm
Copy link
Collaborator Author

avsm commented Jan 10, 2017

But these are alpine images.

I've been migrating the Alpine images to use a builtin solver since Alpine 3.5 -- there was a brief period where they were using the cloud solver too (and pre Alpine 3.5 images still use the cloud solver since there is no backport). More at ocaml/opam#2809 (comment)

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.

None yet

4 participants