Fix inference of gh URIs #358
Conversation
|
Hmm as a matter of fact, I might rename |
|
@gpetiot could you try this on a dummy package or even on ocamlformat see if it improves the situation and/or if you can spot any bug that isn't there on master? I tested myself on a package of mine but I'd prefer to err on the safe side here! |
I keep hitting this issue: $ dune-release opam submit
[-] Submitting
[-] Preparing pull request to ocaml/opam-repository
[-] Fetching https://github.com/ocaml/opam-repository.git#master
[-] Checking out a local release-test-dune-release-v0.0.0 branch
dune-release: [ERROR] /home/gpe/opam-repository/_build/test-dune-release.0.0.0 does not exist, did you run:
dune-release opam pkg -p test-dune-releasealthough (my conf: |
|
Yeah that's the next bug on my todo! @pitag-ha have you had a chance to take a look/test this? |
There was a problem hiding this comment.
Thanks, that makes that part so much more straight-forward!
This PR fixes a bug we recently introduced where the logic to extract a Github repository when needed was broken and would only consider the homepage in most cases instead of falling back to the
dev-repowhenhomepagewasn't on github.
I'm not being able to reproduce that. For me it works both on master and on this branch, if homepage doesn't exist, but dev-repo does. Is that expected?
bin/opam.ml
Outdated
| (match distrib_uri with Some uri -> Ok uri | None -> Pkg.infer_repo_uri pkg) | ||
| >>= Github_uri.get_user_and_repo | ||
| >>= fun (distrib_user, repo) -> | ||
| Pkg.infer_github_repo_uri pkg >>= fun { owner; repo } -> |
There was a problem hiding this comment.
If I'm not missing something, this is a tiny breaking change: someone runs
dune-release opam submit --dist-uri <some_github_uri>
Before, that used to work. Now, it only works, if the distribution uri can be inferred from the opam file. Is that right? Do you think that can be ignored?
There was a problem hiding this comment.
I think this is very very unlikely and that this is inherently broken. To clarify, for that to happen we would need a user whose repo is not hosted on github that happened to have uploaded the distrib tarball somewhere on github. He passes that URI which we then use to determine a user and repo (which won't correspond to the one where the project is hosted, since it's not on github). We then either use them to rewrite the publication message in a github specific way and might also use that to open a PR to opam-repo if they have a broken configuration.
My impression is that would never work because it's using the wrong information in the wrong place.
The only case where it might eventually work is if the project is mirrored on github but even then, I'm not sure it would do the right thing. If we want to support mirroring on github (which we probably should) we'd need an CLI option so one can have the original repo in their opam file but pass the github mirror URI to dune-release so it release to the mirror as it would for a regular github hosted project.
To be perfectly honest I'm not 100% sure myself that I considered all the options but I feel like this has never worked for anyone. Do you have an example in mind of a situation where one would want this behaviour?
Yeah, to trigger the bug you have to have a |
This name would otherwise conflict with the `Uri` external library that we wish to use.
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
NathanReb
force-pushed
the
fix-inference-of-gh-uris
branch
from
8d1097f to
183132f
Compare
CHANGES: ### Added - Add `--no-auto-open` to the default command. It was previously only available for `dune-release opam`. (tarides/dune-release#374, @NathanReb) - Add a `config create` subcommand to create a fresh configuration if you don't have one yet (tarides/dune-release#373, @NathanReb) - Add `--local-repo`, `--remote-repo` and `--opam-repo` options to the default command, they used to be only available for the `opam` subcommand (tarides/dune-release#363, @NathanReb) - Add a `--token` option to `dune-release publish` and `dune-release opam` commands to specify a github token. This allows dune-release to be called through a Github Actions workflow and use the github token through an environment variable. (tarides/dune-release#284 tarides/dune-release#368, @gpetiot @NathanReb) - Log curl calls on verbose/debug mode (tarides/dune-release#281, @gpetiot) - Try to publish the release asset again after it failed (tarides/dune-release#272, @gpetiot) - Improve error reporting of failing git comands (tarides/dune-release#257, @gpetiot) - Suggest a solution for users without ssh setup (tarides/dune-release#304, @pitag-ha) - Allow including git submodules to the distrib tarball by passing the `--include-submodules` flag to `dune-release`, `dune-release bistro` or `dune-release distrib` (tarides/dune-release#300, @NathanReb) - Support 'git://' scheme for dev-repo uri (tarides/dune-release#331, @gpetiot) - Support creation of draft releases and draft PRs. Define a new option `--draft` for `dune-release publish` and `dune-release opam submit` commands. (tarides/dune-release#248, @gpetiot) - Add a new command `check` to check the prerequisites of dune-release and avoid starting a release process that couldn't be finished (tarides/dune-release#318, tarides/dune-release#351, @pitag-ha) - When preparing the opam-repository PR and pushing the local branch to the user's remote opam-repository fork, use `--set-upstream` to ease any further update of the PR (tarides/dune-release#350, @gpetiot) ### Changed - Entirely rely on the remote fork of opam-repository URL in `opam submit` instead of reading the user separately. The information was redundant and could only lead to bugs when unproperly set. (tarides/dune-release#372, @NathanReb) - Use pure token authentication for Github API requests rather than "token as passwords" authentication (tarides/dune-release#369, @NathanReb) - Require tokens earlier in the execution of commands that use the github API. If the token isn't saved to the user's configuration, the prompt for creating one will show up at the command startup rather than on sending the first request (tarides/dune-release#368, @NathanReb) - Attach the changelog to the annotated tag message (tarides/dune-release#283, @gpetiot) - Do not remove versioned files from the tarball anymore. We used to exclude `.gitignore`, `.gitattributes` and other such files from the archive. (tarides/dune-release#299, @NathanReb) - Don't try to push the tag if it is already present and point to the same ref on the remote. `dune-release` must guess which URI to pass to `git push` and may guess it wrong. This change allows users to push the tag manually to avoid using that code. (tarides/dune-release#219, @Julow) - Don't try to create the release if it is already present and points to the same tag (tarides/dune-release#277, @kit-ty-kate) - Recursively exclude all `.git`/`.hg` files and folders from the distrib tarball (tarides/dune-release#300, @NathanReb) - Make the automatic dune-release workflow to stop if a step exits with a non-zero code (tarides/dune-release#332, @gpetiot) - Make git-related mdx tests more robust in unusual environments (tarides/dune-release#334, @sternenseemann) - Set the default tag message to "Release <tag>" instead of "Distribution <tag>" - Opam file linter: check for `synopsis` instead of `description` (tarides/dune-release#291, @kit-ty-kate) - Upgrade the use of the opam libraries to opam 2.1 (tarides/dune-release#343, @kit-ty-kate) ### Deprecated - Deprecate the `--user` CLI options and configuration field, they were redundant with the remote-repo option and field and could be set unproperly, leading to bugs (tarides/dune-release#372, @NathanReb) - Deprecate the use of delegates in `dune-release publish` (tarides/dune-release#276, tarides/dune-release#302, @pitag-ha) - Deprecate the use of opam file format 1.x (tarides/dune-release#352, @NathanReb) ### Removed - Option --name is removed from all commands. When used with `dune-release distrib`, it was previously effectively ignored. Now it is required to add a `(name <name>)` stanza to `dune-project`. (tarides/dune-release#327, @lehy) ### Fixed - Fix a bug where `opam submit` would look up a config file, even though all the required information was provided on the command line. This would lead to starting the interactive config creation quizz if that file did not exist which made it impossible to use it in a CI for instance. (tarides/dune-release#373, @NathanReb) - Fix a bug where `opam submit` would fail on non-github repositories if the user had no configuration file (tarides/dune-release#372, @NathanReb) - Fix a bug where subcommands wouldn't properly read the token files, leading to authentication failures on API requests (tarides/dune-release#368, @NathanReb) - Fix a bug in `opam submit` preventing non-github users to create the opam-repo PR via dune-release. (tarides/dune-release#359, @NathanReb) - Fix a bug where `opam submit` would try to parse the custom URI provided through `--distrib-uri` as a github repo URI instead of using the dev-repo (tarides/dune-release#358, @NathanReb) - Fix the priority of the `--distrib-uri` option in `dune-release opam pkg`. It used to have lower precendence than the url file written by `dune-release publish` and therefore made it impossible to overwrite it if needed. (tarides/dune-release#255, @NathanReb) - Fix a bug with --distrib-file in `dune-release opam pkg` where you would need the regular dune-release generated archive to be around even though you specified a custom distrib archive file. (tarides/dune-release#255, @NathanReb) - Use int64 for timestamps. (tarides/dune-release#261, @gpetiot) - Define the order of packages (tarides/dune-release#263, @gpetiot) - Allow the dry-run mode to continue even after some API call's response were expected by using placeholder values (tarides/dune-release#262, @gpetiot) - Build and run tests for all selected packages when checking distribution tarball (tarides/dune-release#266, @NathanReb) - Improve trimming of the changelog to preserve the indentation of the list of changes. (tarides/dune-release#268, @gpetiot) - Trim the data of the `url` file before filling the `url.src` field. This fixes an issue that caused the `url.src` field to be a multi-line string instead of single line. (tarides/dune-release#270, @gpetiot) - Fix a bug causing dune-release to exclude all hidden files and folders (starting with `.`) at the repository from the distrib archive (tarides/dune-release#298, @NathanReb) - Better report GitHub API errors, all of the error messages reported by the GitHub API are now checked and reported to the user. (tarides/dune-release#290, @gpetiot) - Fix error message when `dune-release tag` cannot guess the project name (tarides/dune-release#319, @lehy) - Always warn about uncommitted changes at the start of `dune-release distrib` (tarides/dune-release#325, @lehy). Otherwise uncommitted changes to dune-project would be silently ignored by `dune-release distrib`. - Fix rewriting of github references in changelog (tarides/dune-release#330, @gpetiot) - Fixes a bug under cygwin where dune-release was unable to find the commit hash corresponding to the release tag (tarides/dune-release#329, @gpetiot) - Fixes release names by explicitly setting it to match the released version (tarides/dune-release#338, @NathanReb) - Fix a bug that prevented release of a package whose version number contains invalid characters for a git branch. The git branch names are now sanitized. (tarides/dune-release#271, @gpetiot) - `publish`: Fix the process of inferring user name and repo from the dev repo uri (tarides/dune-release#348, @pitag-ha)
This PR fixes a bug we recently introduced where the logic to extract a Github repository when needed was broken and would only consider the homepage in most cases instead of falling back to the
dev-repowhenhomepagewasn't on github.The fact that we were simply passing strings around wasn't helping so I extracted a type for github URIs (that we could in the future turn as representation for github repos really).
This fixes the bug I was running into when testing the latest dune-release before attempting to release 1.5.
While doing this work I realized there were a few things we were doing wrong.
The first one was that we were trying to parse the user provided
--distrib-urias a github URI which is inherently wrong since the fact that the user needs to provide the distrib-uri on its own most likely means he's not releasing on github. I fixed that by removing this logic entirely as in those cases it was clear we were dependent on the repo being hosted on github so we're now using the dev-repo/homepage directly. I fixed this here as it was logical and simpler to do so as part of the refactoring.Another bug that I will fix in a subsequent PR is that
opam submitwill fail if we can't find a github repo associated with the package. It shouldn't be the case and non github users should still be able to release using dune-release at this point.Let me know what you think!