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

fix: linter warnings #324

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jdrouet
Copy link

@jdrouet jdrouet commented Sep 19, 2024

When compiling the linter complained that php8x was not defined. I fixed it using rustc-check-cfg as defined here.

The compiler also complained that cfg(docs) was invalid. It's because it should be cfg(doc). I updated that. After updating, the compiler mentioned that feature(doc_cfg) was only available in nightly so I update the embed Dockerfile to reflect that.

@jdrouet jdrouet changed the title fix linter warnings fix: linter warnings Sep 19, 2024
@@ -10,6 +10,6 @@ ENV RUSTUP_HOME=/rust
ENV CARGO_HOME=/cargo
ENV PATH=/cargo/bin:/rust/bin:$PATH

RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --no-modify-path
RUN (curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --default-toolchain nightly --no-modify-path) && rustup default nightly
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nightly is really required here?

Copy link
Author

Choose a reason for hiding this comment

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

It seems like feature(doc_cfg) is only available in nightly

Copy link
Author

Choose a reason for hiding this comment

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

error[E0554]: `#![feature]` may not be used on the stable release channel
 --> src/lib.rs:6:18
  |
6 | #![cfg_attr(doc, feature(doc_cfg))]
  |                  ^^^^^^^^^^^^^^^^

error: aborting due to 1 previous error

@jdrouet
Copy link
Author

jdrouet commented Sep 30, 2024

@ptondereau any idea what could be a plan to move on with this?

@ptondereau
Copy link
Collaborator

Hey, thank you for your patience, I'm not the lead maintainer for now and as I don't work with PHP, I try my best to answer question 😅
the CI seems broken here but anyway, for the Dockerfile, is there a way to rely on nightly only for the doc problem? Because, it executes the test as well with nightly.
cc @danog

@jdrouet
Copy link
Author

jdrouet commented Oct 1, 2024

@ptondereau that's exactly what this PR does: only use nightly 😉

@Xenira
Copy link
Collaborator

Xenira commented Oct 7, 2024

Some of this is also solved in #320 as I needed to make clippy happy.

@jdrouet
Copy link
Author

jdrouet commented Oct 7, 2024

@Xenira yeah, but considering @danog didn't seem to approve your changes, I went for a smaller change so that we can iterate.

@Xenira
Copy link
Collaborator

Xenira commented Oct 7, 2024

@jdrouet ye, just wanted to make sure you've seen it.

tbh i don't know what to change to get it merged. Did the bare minimum to get ci woking again.

@jdrouet
Copy link
Author

jdrouet commented Oct 9, 2024

Agreed. I don't know if this repository will move forward at anytime soon.
Maybe @danog or @davidcole1340 would consider moving this repository to an org with more people that could contribute?

@Xenira
Copy link
Collaborator

Xenira commented Oct 9, 2024

If it doesn't Ill probably create my own repo.
Have a bunch of changes locally that I would like to contribute, but am blocked by CI and can't wait 2 months on each of those.

No hard feelings towards the maintainers. Just want to move forward.

@davidcole1340
Copy link
Owner

I'm happy to move this repo out into an organization - will do this later this week, but if you think there should be more approvers/maintainers please reach out to me! #140

Copy link
Owner

@davidcole1340 davidcole1340 left a comment

Choose a reason for hiding this comment

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

Looks good, but do you know why CI is failing on this PR? Will merge after it passes

@Xenira
Copy link
Collaborator

Xenira commented Oct 10, 2024

Regarding CI:

  1. Stable and unstable are tested in CI. These changes only work on unstable (https://github.com/davidcole1340/ext-php-rs/actions/runs/10935703203/job/30365113910?pr=324#step:12:256)
  2. Macos will also fail because of clang/llvm (ci: fix pipeline #320)

@jdrouet
Copy link
Author

jdrouet commented Oct 10, 2024

I think on common issue could be the use of unstable features. Removing those would fix some CI issues.

@davidcole1340
Copy link
Owner

@jdrouet I rebased your branch on master, please review that the logic is still correct

@Xenira
Copy link
Collaborator

Xenira commented Oct 21, 2024

Current CI failures are caused by #331

…ests

Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
@Xenira
Copy link
Collaborator

Xenira commented Oct 21, 2024

@jdrouet rebased with working master pipeline

@Xenira
Copy link
Collaborator

Xenira commented Oct 21, 2024

Without looking further into this and correct me if I am wrong:

Merging this would mean we loose stable support. Would like to avoid that. Guess same point as #324 (comment) made.

@jdrouet
Copy link
Author

jdrouet commented Oct 22, 2024

@Xenira the problem is that this crate has many dependencies on nightly features. This introduces some errors at build time and makes it harded to reproduce the build. It can also be unstable over time considering the nightly features are, by definition, not sure to remain the same or land in stable. I'd suggest to remove them to only rely on stable features. I can do it if you agree with that.

@Xenira
Copy link
Collaborator

Xenira commented Oct 22, 2024

@jdrouet maybe I don't understand the problem, but what exactly is not working with stable right now.

master is building just fine using the stable channel.

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.

4 participants