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

Add -lbcrypt #156

Merged
merged 8 commits into from
Dec 14, 2021
Merged

Add -lbcrypt #156

merged 8 commits into from
Dec 14, 2021

Conversation

yutannihilation
Copy link
Contributor

Close extendr/extendr#339

As of v1.57.0, Rust requires bcrypt to use BCryptGenRandom (Before then, it used to work without bcrypt to support Windows XP).

c.f. rust-lang/rust#84096

@yutannihilation
Copy link
Contributor Author

I didn't notice this would affect the snapshot test. Sorry for my laziness...

@yutannihilation
Copy link
Contributor Author

The failure on R-devel ubuntu runner looks unrelated to this change. Probably something related to line breaks is wrong on testthat's side. For example:

── Failure (test-knitr-engine.R:27:3): Snapshot test of knitr-engine ───────────
Snapshot of code has changed:
old[6:23] vs new[6:14]
  "  "
  "  "
  "  Basic use example:"
  "  "
+ "  library(rextendr)# create a Rust functionrust_function(\"fn add(a:f64, b:f64) -> f64 { a + b }\")# call it from Radd(2.5, 4.7)#> [1] 7.2"
  "  "
- "  ```r"
- "  library(rextendr)"
  "  "
- "  # create a Rust function"
- "  rust_function(\"fn add(a:f64, b:f64) -> f64 { a + b }\")"
- "  "
- "  # call it from R"
- "  add(2.5, 4.7)"
- "  #> [1] 7.2"
- "  ```"
- "  "
  "  The package also enables a new chunk type for knitr, `extendr`, which compiles and evaluates Rust code. For example, a code chunk such as this one:"
  "  ````markdown"
  "  ```{extendr}"

@Ilia-Kosenkov
Copy link
Member

Should we perhaps fix snapshot test as well?

@yutannihilation
Copy link
Contributor Author

What do you mean? As you can see, nothing is different except for that the result with R-devel is without line breaks.

Actually, it seems the latest version of testthat's snapshot feature is broken somehow, though I still don't understand the details...

r-lib/testthat#1509

@yutannihilation
Copy link
Contributor Author

Hmm, it seems our problem is not related to r-lib/testthat#1509...

@Ilia-Kosenkov
Copy link
Member

What do you mean? As you can see, nothing is different except for that the result with R-devel is without line breaks.

Sorry I was reading from my phone and did not realize what the actual problem with the snapshot was.
What would be our strategy? Allow this one to fail for now? I may be able to get a devel version installed locally and see what happens to snapshot tests' output.

@yutannihilation
Copy link
Contributor Author

Ah, I see. Sorry I didn't describe the details in the above comment.

What would be our strategy? Allow this one to fail for now?

I have no idea at the moment... Let me think.

@Ilia-Kosenkov
Copy link
Member

@clauswilke, would appreciate your opinion as well.

@Ilia-Kosenkov
Copy link
Member

Perhaps we can file an issue at {testthat}? We have a reproducible case, maybe they can help us?

@yutannihilation
Copy link
Contributor Author

Here's a minimal reproducible example repo. It seems the problem is around cat_file(), which we implemented, so it might not be where testthat is responsible for.

https://github.com/yutannihilation/testTestthat

But, really weird thing is, if I switch the GitHub action setting to the latest check-pak, it won't fail. I have no idea. Both use rcmdcheck::rcmdcheck(), so I have no idea why the results are different...

@yutannihilation
Copy link
Contributor Author

I'm still not sure about the root cause of the failure, but it seems the new workflow works so this pull request should be ready to merge. While we can keep investigating what was happening, things on R-devel changes fast, so I think it might not be worth the effort.

Copy link
Member

@Ilia-Kosenkov Ilia-Kosenkov left a comment

Choose a reason for hiding this comment

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

Perhaps we should indeed rely on r-tools to find dependencies and check package instead of doing it ourselves. LGTM, at least for now.

@yutannihilation
Copy link
Contributor Author

I see, thanks. IMHO, if there's now warning using -lbcrypt with older Rust, we don't need to do anything, but we'll need to check. I'll file a new issue for this.

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.

Was something changed on Windows setup after Rust 1.57.0?
2 participants