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

deprecate deno or improve maintainability #38

Open
Xuanwo opened this issue Jun 29, 2024 · 12 comments · Fixed by #39
Open

deprecate deno or improve maintainability #38

Xuanwo opened this issue Jun 29, 2024 · 12 comments · Fixed by #39
Labels
help wanted Extra attention is needed

Comments

@Xuanwo
Copy link
Collaborator

Xuanwo commented Jun 29, 2024

Looks like deno did some breaking changes:

error[E0308]: mismatched types
   --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_core-0.272.0/arena/raw_arena.rs:290:43
    |
290 |         for alloc in BitSet::from_bit_vec(vacant).into_iter() {
    |                      -------------------- ^^^^^^ expected `bit_vec::BitVec`, found `BitVec`
    |                      |
    |                      arguments to this function are incorrect
    |
    = note: `BitVec` and `bit_vec::BitVec` have similar names, but are actually distinct types
note: `BitVec` is defined in crate `bit_vec`
   --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/bit-vec-0.7.0/src/lib.rs:245:1
    |
245 | pub struct BitVec<B = u32> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: `bit_vec::BitVec` is defined in crate `bit_vec`
@Xuanwo Xuanwo added good first issue Good for newcomers help wanted Extra attention is needed labels Jun 29, 2024
@PsiACE
Copy link
Contributor

PsiACE commented Jun 29, 2024

It's just that bit-vec released a new version

contain-rs/bit-set#32

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Jun 29, 2024

It's just that bit-vec released a new version

deno specify bit-vec = "0" ...

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Jun 29, 2024

cc @wangrunji0408, #39 is the temp workaround. To fully address this, we need either:

  • A. Wait for deno bumps all it's dpendences with correct version instead of 0
  • B. Commit Cargo.lock in repo to make sure to pick the correct version during build
  • C. Add some cargo update in CI to pick the version.
  • D. Drop deno support entirely (?)

What do you think?

@Xuanwo Xuanwo closed this as completed in 9d1a9a6 Jun 29, 2024
@Xuanwo Xuanwo reopened this Jun 29, 2024
@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Jun 29, 2024

Reopen to continue the discussion.

@wangrunji0408
Copy link
Contributor

wangrunji0408 commented Jun 29, 2024

Let's keep our patch until deno-core releases its fix.

@PsiACE
Copy link
Contributor

PsiACE commented Jun 29, 2024

bit-set released version 0.6.0 🥰

@BugenZhao
Copy link
Member

BugenZhao commented Jul 23, 2024

Personally I feel like deno is not that serious when it comes to specifying dependencies, e.g., foo = "0" or bar = "=x.y.z" with no strong reason. This may be because it is more commonly distributed as a binary than being used as a library. As a result, it is challenging for us to maintain support for it in this project, and it also blocks bumping other dependencies in the downstream (risingwavelabs/risingwave#13695 (comment)).

D. Drop deno support entirely (?)

So I would like to vote for this option. In risingwavelabs/risingwave#17544 @MrCroxx is also going to drop the support of Deno UDF in RisingWave.

@BugenZhao
Copy link
Member

Let's keep our patch until deno-core releases its fix.

There's a considerable amount of breaking changes in recent releases of deno. I'm afraid it requires a lot of effort to stay up to date with the upstream. 😕

@xxchan xxchan removed the good first issue Good for newcomers label Jul 23, 2024
@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Jul 23, 2024

Yes, I feel the same way; maintaining deno support for arrow-udf isn't worth it unless we have users.

@MrCroxx
Copy link

MrCroxx commented Jul 23, 2024

+1 for removing it until we get a real user.

@wangrunji0408
Copy link
Contributor

cc @bakjos for awareness. What's your opinion? Is it possible to link a prebuilt binary of deno?

@bakjos
Copy link
Contributor

bakjos commented Jul 23, 2024

@wangrunji0408 yes, think it's possible, but one of the advantages of having it embedded is less serialization and less communication overhead. I'm in long break, but can take a look in 6 weeks from now

@xxchan xxchan changed the title ci: breaking changes in deno deprecate deno or improve maintainability Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants