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

[SPARK-30737][SPARK-27262][R][BUILD] Reenable CRAN check with UTF-8 encoding to DESCRIPTION #27472

Closed
wants to merge 2 commits into from

Conversation

HyukjinKwon
Copy link
Member

What changes were proposed in this pull request?

This PR proposes to reenable CRAN check disabled at #27460. Given the tests #27468, seems we should also port #23823 together.

Why are the changes needed?

To check CRAN back.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

It was tested at #27468 and Jenkins should test it out.

@SparkQA
Copy link

SparkQA commented Feb 6, 2020

Test build #117960 has finished for PR 27472 at commit a613fb4.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

# - It is strongly recommended to set environment variable TZ to
# America/Los_Angeles (or equivalent)
suppressWarnings(expect_equal(DF$time[1], as.POSIXlt("2016-01-10")))
expect_equal(DF$time[1], as.POSIXlt("2016-01-10"))
Copy link
Member Author

Choose a reason for hiding this comment

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

This PR piggybacks one fix here. It's just rather a nit fix to don't suppress warnings anymore introduced from #27460 to make tests pass.

@@ -62,3 +62,4 @@ Collate:
RoxygenNote: 5.0.1
VignetteBuilder: knitr
NeedsCompilation: no
Encoding: UTF-8
Copy link
Member Author

Choose a reason for hiding this comment

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

This one, we had to revert before due to Jekins enviornment WIP. I think now it's all good to go.

@HyukjinKwon
Copy link
Member Author

@shivaram and @felixcheung, we will reenable CRAN here.

Copy link
Contributor

@shivaram shivaram left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @HyukjinKwon

HyukjinKwon added a commit that referenced this pull request Feb 6, 2020
…ncoding to DESCRIPTION

### What changes were proposed in this pull request?

This PR proposes to reenable CRAN check disabled at #27460. Given the tests #27468, seems we should also port #23823 together.

### Why are the changes needed?
To check CRAN back.

### Does this PR introduce any user-facing change?
No.

### How was this patch tested?
It was tested at #27468 and Jenkins should test it out.

Closes #27472 from HyukjinKwon/SPARK-30737.

Authored-by: HyukjinKwon <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
(cherry picked from commit b95ccb1)
Signed-off-by: HyukjinKwon <[email protected]>
HyukjinKwon added a commit that referenced this pull request Feb 6, 2020
…ncoding to DESCRIPTION

### What changes were proposed in this pull request?

This PR proposes to reenable CRAN check disabled at #27460. Given the tests #27468, seems we should also port #23823 together.

### Why are the changes needed?
To check CRAN back.

### Does this PR introduce any user-facing change?
No.

### How was this patch tested?
It was tested at #27468 and Jenkins should test it out.

Closes #27472 from HyukjinKwon/SPARK-30737.

Authored-by: HyukjinKwon <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
(cherry picked from commit b95ccb1)
Signed-off-by: HyukjinKwon <[email protected]>
@HyukjinKwon
Copy link
Member Author

Merged to master, branch-3.0, and branch-2.4.

Thanks @shivaram

@dongjoon-hyun
Copy link
Member

Great! Late LGTM.

@shaneknapp
Copy link
Contributor

LGTM, and thanks @HyukjinKwon !

@zero323
Copy link
Member

zero323 commented Feb 6, 2020

Could maybe start talking about moving to Roxygen2 7.0.1, pretty please?

(Documentation seems to build cleanly across all supported versions of R, with most of the files, including most of the binaries. There are no more warnings. SPARK-22430 has been resolved long time ago).

@HyukjinKwon
Copy link
Member Author

That's probably less urgent but yeah we should do it. @zero323 what about creating a JIRA?

@zero323
Copy link
Member

zero323 commented Feb 6, 2020

That's probably less urgent but yeah we should do it. @zero323 what about creating a JIRA?

Yes, it is not urgent. It is mostly annoyance at this point. The reason I even ask, is that it has similar set of dependencies to testthat (I think that rlang is the primary concern), so keeping these two more or less synced, might extend lifespan of the current setup.

@shaneknapp
Copy link
Contributor

Yes, it is not urgent. It is mostly annoyance at this point. The reason I even ask, is that it has similar set of dependencies to testthat (I think that rlang is the primary concern), so keeping these two more or less synced, might extend lifespan of the current setup.

@zero323 -- when you create the jira, assign it to me pls. thanks!

@zero323
Copy link
Member

zero323 commented Feb 6, 2020

@shaneknapp I don't think I have permissions to do that, but here's the ticket ‒ SPARK-30747.

@HyukjinKwon HyukjinKwon deleted the SPARK-30737 branch March 3, 2020 01:16
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.

6 participants