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 warning on Windows. #6883

Merged
merged 1 commit into from
Apr 21, 2021
Merged

Conversation

trivialfis
Copy link
Member

@trivialfis trivialfis commented Apr 21, 2021

Targeted at 1.4 branch, will port to master once merged. Address warning found by @hetong007 in #6793 (comment) .

Do not merge until all tests on R-hub have passed. (Huge thanks to R-hub). Progress can be tracked here:

@trivialfis
Copy link
Member Author

@hetong007 Hi, I ran some tests as listed in the PR description. There are 2 failures, one is on Solaris + Oracle Developer Studio 12.6, another one is due to the indirect dependency on readr (PREPERROR). Others are passing.

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2021

Codecov Report

Merging #6883 (c7b94e6) into release_1.4.0 (a347ef7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##           release_1.4.0    #6883   +/-   ##
==============================================
  Coverage          81.80%   81.80%           
==============================================
  Files                 13       13           
  Lines               3852     3852           
==============================================
  Hits                3151     3151           
  Misses               701      701           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a347ef7...c7b94e6. Read the comment docs.

@hetong007
Copy link
Member

@trivialfis Thanks for the efforts! I think the Solaris + Oracle Developer Studio 12.6 environment is challenging, and based on my experiences from the last submission, errors/warnings on this environment is not the same as the CRAN machine, thus I'd say it could be just fine. And the readr dependency should be fine as well, exactly as you mentioned.

I'm seeing some CI failures, do you suggest me to wait for an update, or should I just apply the current patch and re-submit?

@Craigacp
Copy link
Contributor

I can't see the Solaris error, when I went to those links it looked like everything worked (but they were both using GCC 5), but I've got experience working on Solaris 11 with Developer Studio 12.6 so I might be able to take a look at some point. We had XGBoost running on Solaris 11 SPARC several years ago.

Is there a link to the actual failure somewhere?

@trivialfis
Copy link
Member Author

trivialfis commented Apr 21, 2021

@hetong007 I re-ran the test, the previous failure seems to be internal to GitHub action.

The test on centos 8 seems to be related to LaTeX output, I got confused by the long expanded macro from the boost library.

12845#> Loading required package: knitr
12846#> Error: processing vignette 'xgboost.Rnw' failed with diagnostics:
12847#> Running 'texi2dvi' on 'xgboost.tex' failed.
12848#> LaTeX errors:
12849#> ! LaTeX Error: File `framed.sty' not found.
12850#> Type X to quit or to proceed,
12851#> or enter new name. (Default extension: sty)
12852#> ! Emergency stop.

But I think cran should have a working TeX distribution.

@Craigacp Sorry those are the public links from R-hub. I receive email notification for each run, here are the artifacts containing build logs:

@trivialfis
Copy link
Member Author

trivialfis commented Apr 21, 2021

@hetong007 I will merge this PR into the 1.4 branch. Feel free to fetch from this branch.

Feel free to ping me in #6793 if there's anything else I can help.

@trivialfis trivialfis merged commit a6d1fbf into dmlc:release_1.4.0 Apr 21, 2021
@trivialfis trivialfis deleted the 1.4-fix-R branch April 21, 2021 18:57
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.

5 participants