Skip to content

Fix a type error. Fix the binary build script - #10

Open
vtrofin wants to merge 4 commits into
gaku-sei:mainfrom
vtrofin:type-fixes
Open

Fix a type error. Fix the binary build script#10
vtrofin wants to merge 4 commits into
gaku-sei:mainfrom
vtrofin:type-fixes

Conversation

@vtrofin

@vtrofin vtrofin commented Apr 20, 2023

Copy link
Copy Markdown

@gaku-sei i'm opening a PR here because I can't create issues on your gaku-sei/pyaco repo.

  1. I got this error in my local env and i've pushed a change for it. Hope it's a good fix.
error[E0432]: unresolved import `notify::ReadDirectoryChangesWatcher`
 --> pyaco-core/src/notify.rs:3:5
  |
3 | use notify::ReadDirectoryChangesWatcher;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `ReadDirectoryChangesWatcher` in the root
  |
help: consider importing this variant instead
  |
3 | use notify::WatcherKind::ReadDirectoryChangesWatcher;
  |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  1. Update makefile to handle Mac M1 builds. The release workflow worked fine on my computer

@vtrofin

vtrofin commented Apr 20, 2023

Copy link
Copy Markdown
Author

I'll post some questions below about some issues i'm having in understanding the old original code, the one in the scoville/tailwind-generator repo. Let's leave this PR open and have a chat in here :)

@vtrofin
vtrofin marked this pull request as draft April 20, 2023 02:10
@vtrofin

vtrofin commented Apr 20, 2023

Copy link
Copy Markdown
Author
  1. The main issue i'm having is compiling the original tailwind-generator locally. My goal is to have properly understood your initial code from Scoville in order to better understand the changes you made in your last 3 PRs.

So, this is your "original code" in the gaku-sei/pyaco repo regarding the get! macro. Once I apply the type fixes in this PR, i can properly compile the pyaco-node package

➜  gakusei-tailwind-generator git:(main) cargo check --package pyaco-node
    Checking pyaco-core v0.1.0 (/Users/victor_trofin/github/gakusei-tailwind-generator/pyaco-core)
    Checking pyaco-validate v0.1.0 (/Users/victor_trofin/github/gakusei-tailwind-generator/pyaco-validate)
    Checking pyaco-generate v0.1.0 (/Users/victor_trofin/github/gakusei-tailwind-generator/pyaco-generate)
    Checking pyaco-node v0.1.0 (/Users/victor_trofin/github/gakusei-tailwind-generator/pyaco-node)
    Finished dev [unoptimized + debuginfo] target(s) in 1.66s

This is the old code in scoville/tailwind-generator which i'm trying to compile. And I get the following errors. The two pieces of code are absolutely identical but I get errors

error[E0282]: type annotations needed
  --> pyaco-node/src/lib.rs:20:14
   |
20 |             .downcast_or_throw::<$type, _>(&mut $cx)?
   |              ^^^^^^^^^^^^^^^^^ cannot infer type
...
32 |     let input = get!(cx, options, "input");
   |                 -------------------------- in this macro invocation
   |
   = note: this error originates in the macro `get` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0599]: no method named `downcast_or_throw` found for struct `Handle<'_, _>` in the current scope
   --> pyaco-node/src/lib.rs:20:14
    |
20  |             .downcast_or_throw::<$type, _>(&mut $cx)?
    |              ^^^^^^^^^^^^^^^^^ method not found in `Handle<'_, _>`
...
32  |     let input = get!(cx, options, "input");
    |                 -------------------------- in this macro invocation
    |
   ::: /Users/victor_trofin/.cargo/registry/src/github.com-1ecc6299db9ec823/neon-0.10.1/src/handle/mod.rs:245:12
    |
245 |     pub fn downcast_or_throw<'b, U: Value, C: Context<'b>>(&self, cx: &mut C) -> JsResult<'a, U> {
    |            ----------------- the method is available for `neon::handle::Handle<'_, _>` here
    |
    = note: the method was found for
            - `neon::handle::Handle<'a, T>`
    = note: this error originates in the macro `get` (in Nightly builds, run with -Z macro-backtrace for more info)


I am unable to fix this compile error no matter what i've tried. Any idea on how I could fix it?

@gaku-sei

gaku-sei commented Apr 20, 2023

Copy link
Copy Markdown
Owner

Hey @vtrofin and thank you for the heads up.

Indeed, while the code compile just fine on Windows It'll fail on other platforms as ReadDirectoryChangesWatcher is only for Windows. The good news is that notify has a dedicated type alias for that: https://docs.rs/notify/5.1.0/notify/type.RecommendedWatcher.html that's resolved at compile time: https://docs.rs/notify/5.1.0/src/notify/lib.rs.html#343 if you don't mind using this type instead 👍

@gaku-sei

gaku-sei commented Apr 20, 2023

Copy link
Copy Markdown
Owner

So, this is your "original code" in the gaku-sei/pyaco repo regarding the get! macro. Once I apply the type fixes in this PR, i can properly compile the pyaco-node package

If you refer to the code that was on my repo before the last couple of prs, notice that the code is not the same as Scoville's (if my memory serves me well). Also, this crate now requires Rust 1.68 (soon 1.68.2) to compile, and if you try to compile it with this version of Rust you might get different compile errors reported than the version used at the time.

Also, that that macro got dropped and Neon replaced by Napi (which I find paradoxally more user friendly yet more barebone 😅).

As for the error itself, notice that downcast_or_throw is implemented for Handle with this constraint: impl<'a, T: Value> Handle<'a, T> { ... } (where the Value trait is here. Since it seems the compiler fails to infer the type of the generic, it's normal it doesn't know this type implements Value, and though, you get 2 errors for the price of one!

Now, what's interesting is to see where that error comes from: Neon got a patch update from 0.10.0-alpha.-* (the version we used to use) and 0.10.0 and 0.10.1 (the version that cargo downloaded since the version specified is 0.10) that's breaking.

0.10.0-alpha.1: https://docs.rs/neon/0.10.0-alpha.1/neon/object/trait.Object.html#method.get
0.10.1: https://docs.rs/neon/0.10.1/neon/object/trait.Object.html#method.get

get used to be like: "I'll return an opaque JsValue and let you do the hard work" when the new versions says "I will not only get the value but also downcast it".

The fix could look like this:

macro_rules! get {
    ($cx:ident, $options:ident, $name:expr, $type:ty) => {
        let value = options
            .get::<$type, _, _>(&mut $cx, $name)?;
        value
    };

    ($cx:ident, $options:ident, $name:expr) => {
        get!($cx, $options, $name, JsString)
    };
}

But at that point the macro is useless (and actually always has been) and I would recommend dropping it.

You can deref the returned value or take ownership:

let value = get!(...);
let value_ref = *value; // will be &String for a String
let value_owned = value.to_owned(); // will be String for, well, String, implies a clone

I would also pin the version in the Cargo.toml for this crate.

Side note, here is a reduction of the macro using only Rust's std: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=bcd20fe60189b4784e7a779a5cf0b88e

@vtrofin

vtrofin commented Apr 27, 2023

Copy link
Copy Markdown
Author

Thank you for your suggestions @gaku-sei . I ended up reusing the options.get().value() throughout the file

 let capture_regex = options
        .get::<JsString, FunctionContext, _>(&mut cx, "captureRegex")?
        .value(&mut cx);

I also made the suggested type update and added some updates to the release workflow in order to handle Mac M1s. Ready for your review.

@vtrofin
vtrofin marked this pull request as ready for review April 27, 2023 03:54
@vtrofin vtrofin changed the title Fix a type error. A million other questions Fix a type error. Fix the build script Apr 28, 2023
@vtrofin vtrofin changed the title Fix a type error. Fix the build script Fix a type error. Fix the binary build script Apr 28, 2023
Comment thread Makefile
@if ! command -v cross &> /dev/null; then \
cargo install cross --git https://github.com/cross-rs/cross; \
fi
yarn

@vtrofin vtrofin Apr 28, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

could be improved to first check whether yarn is installed and if not use npm -i instead of yarn

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.

2 participants