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

WIP: Fix publish workflow #304

Closed
wants to merge 1 commit into from
Closed

Conversation

dblock
Copy link
Contributor

@dblock dblock commented Sep 1, 2021

Attempts at fixing the publish workflow.

@dblock
Copy link
Contributor Author

dblock commented Sep 1, 2021

@lebensterben Hit approve on the workflow so we can see it go to green?

@lebensterben
Copy link
Member

@dblock
I'm not a maintainer. It need a maintainer to start workflows for first-time contributors.

@dblock
Copy link
Contributor Author

dblock commented Sep 2, 2021

Two problems remaining with the build.

I enabled CI to build on all branches, don't see any reason not to. It also makes it possible for contributors to run actions on their forks.

  1. I fixed most clippy errors, but can't figure out how to deal with this one. Suppressed it for now. These are tuples, so |-ing them seems correct. What am I missing?
$ cargo clippy --all-targets -- --deny warnings
    Checking lychee-lib v0.7.0 (/Users/dblock/source/lychee/dblock/lychee-lib)
error: unnested or-patterns
   --> lychee-lib/src/extract.rs:126:5
    |
126 | /     matches!(
127 | |         (attr_name, elem_name),
128 | |         ("href", _)
129 | |             | ("src", _)
...   |
133 | |             | ("onhashchange", "body")
134 | |     )
    | |_____^
    |
    = note: `-D clippy::unnested-or-patterns` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns
    = note: this error originates in the macro `matches` (in Nightly builds, run with -Z macro-backtrace for more info)
  1. On Linux (in CI), cargo publish --dry-run --manifest-path lychee-bin/Cargo.toml is failing to use the source. See https://github.com/dblock/lychee/runs/3495547843?check_suite_focus=true. Seems to work on mac. I tried publishing locally or doing all kinds of things to lychee-lib, but lychee-bin just won't pickup that output (I incremented version to 0.7.1 and it keeps only seeing 0.7.0 available).
error[E0432]: unresolved import `lychee_lib::collector::Collector`
  --> src/main.rs:72:17
   |
72 |     collector::{Collector, Input},
   |                 ^^^^^^^^^ no `Collector` in `collector`

It looks like this is supposed to be this way and only published dependencies can only be picked up. I tried using cargo-publish-all that creates a workspace and seems to be designed to solve this, but end up hitting idanarye/rust-typed-builder#57

@dblock dblock force-pushed the fix-tests branch 2 times, most recently from 2c5b54c to efc8ada Compare September 2, 2021 12:26
@dblock dblock changed the title Fix: remove URL that is currently returning a 503. Fix: remove URL that is currently returning a 503, fix build Sep 2, 2021
@mre
Copy link
Member

mre commented Sep 3, 2021

About point no. 1, clippy wants you to write it like this:

    matches!(
        (attr_name, elem_name),
        ("href" | "src" | "srcset" | "cite", _) | ("data", "object") | ("onhashchange", "body")

I fixed this and other linting issues in #306 with c5d7544.
You can cherry-pick that commit if you like.

@dblock
Copy link
Contributor Author

dblock commented Sep 3, 2021

About point no. 1, clippy wants you to write it like this:

    matches!(
        (attr_name, elem_name),
        ("href" | "src" | "srcset" | "cite", _) | ("data", "object") | ("onhashchange", "body")

I fixed this and other linting issues in #306 with c5d7544.
You can cherry-pick that commit if you like.

Oh I see, thx.

Let's fix the build first. What do we do about the publishing checker?

@dblock dblock changed the title Fix: remove URL that is currently returning a 503, fix build WIP: Fix publish workflow Sep 3, 2021
@mre
Copy link
Member

mre commented Sep 3, 2021

What do we do about the publishing checker?

I don't think cargo-publish-all is a good solution for this. It hasn't seen any updates in two years and there are issues similar to the one you described (https://gitlab.com/torkleyy/cargo-publish-all/-/issues/3).
It's also a third-party solution and I think we should focus on core tools like cargo in that case.

My plan is to cherry-pick some changes from this PR and then I would like to close the PR and publish a new version of lychee, which should make future builds green for the time being. In the long run I'd very much like a solution that doesn't require publishing lychee-lib if there's an API change. #305

mre added a commit that referenced this pull request Sep 3, 2021
@mre
Copy link
Member

mre commented Sep 3, 2021

0.7.1 is out. 🥳
@dblock and @lebensterben thanks for your contributions. I'll close this as the CI pipeline is green again. We can talk about workflow improvements in #305.

@mre mre closed this Sep 3, 2021
@dblock
Copy link
Contributor Author

dblock commented Sep 3, 2021

0.7.1 is out. 🥳
@dblock and @lebensterben thanks for your contributions. I'll close this as the CI pipeline is green again. We can talk about workflow improvements in #305.

So the problem with this green pipeline is that it checks publication of lychee-bin against the previous version of lychee. So it's fake green. I got a fix coming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants