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

Upkeep #79

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Upkeep #79

wants to merge 21 commits into from

Conversation

dunkenwg
Copy link
Contributor

No description provided.

@dunkenwg
Copy link
Contributor Author

When I run the check/documentation for this package I get:

══ Documenting ══════════════════════════════════════════════════════════════════════════════════════
ℹ Updating embr documentation
Writing NAMESPACE
ℹ Loading embr
✖ pars.R:105: S3 method `pars.character` needs @export or @exportS3method tag.
Writing NAMESPACE

Should pars.character() be exported?

@dunkenwg dunkenwg requested a review from joethorley September 17, 2024 22:57
Copy link
Member

@joethorley joethorley left a comment

Choose a reason for hiding this comment

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

Can you talk to Ayla when she is back about the NOTE around exporting S3 tags? I don't think it should be exported as it will likely break things in this case but we should suppress the NOTE if we can.

@dunkenwg dunkenwg requested a review from aylapear September 18, 2024 14:44
@dunkenwg
Copy link
Contributor Author

Can you talk to Ayla when she is back about the NOTE around exporting S3 tags? I don't think it should be exported as it will likely break things in this case but we should suppress the NOTE if we can.

I'm also going to wait until Ayla is back to discuss why the Mac OS R-CMD-check is failing due to a segmentation fault as I'm quite lost.

@aylapear
Copy link
Member

Can you talk to Ayla when she is back about the NOTE around exporting S3 tags? I don't think it should be exported as it will likely break things in this case but we should suppress the NOTE if we can.

This seems to be a new thing the documentation is requiring us to do. For now ignore the issue as we are working out what to do for this across the board. Please make an internal-infrastructure issue about this and list this repo in the comments and once we figure out the appropriate fix we can apply it to the relevant repositories.

@aylapear
Copy link
Member

I'm also going to wait until Ayla is back to discuss why the Mac OS R-CMD-check is failing due to a segmentation fault as I'm quite lost.

I am not familiar with this specific issue, give me a call next time you are working on it and we can chat through some things to test to dig into the issue further.

Copy link
Member

@aylapear aylapear left a comment

Choose a reason for hiding this comment

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

Changes look good but we need to figure out the failing action before we can merge in the changes.

@dunkenwg
Copy link
Contributor Author

Check if action runs on no tests then add tests back

@joethorley
Copy link
Member

We are getting a segfault.

I suspect the action is failing when it tries to install JAGS for macos

> test_check("embr")

 *** caught segfault ***
address 0x6e6f432f34366d72, cause 'invalid permissions'

Traceback:
 1: dyn.load(file, DLLpath = DLLpath, ...)
 2: library.dynam(lib, package, package.lib)
 3: loadNamespace(i, c(lib.loc, .libPaths()), versionCheck = vI[[i]])
 4: namespaceImport(ns, loadNamespace(i, c(lib.loc, .libPaths()),     versionCheck = vI[[i]]), from = package)
 5: loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]])
 6: asNamespace(ns)
 7: namespaceImportFrom(ns, loadNamespace(j <- i[[1L]], c(lib.loc,     .libPaths()), versionCheck = vI[[j]]), i[[2L]], from = package)
 8: loadNamespace(i, c(lib.loc, .libPaths()), versionCheck = vI[[i]])
 9: namespaceImport(ns, loadNamespace(i, c(lib.loc, .libPaths()),     versionCheck = vI[[i]]), from = package)
10: loadNamespace(x)
11: ggplot_data(x, y, x_name, y_name)
12: plot_data.factor(x = x[[x_name]], y = x[[y_name]], x_name = x_name,     y_name = y_name)
13: plot_data(x = x[[x_name]], y = x[[y_name]], x_name = x_name,     y_name = y_name)
14: plot_data.data.frame(data)
15: plot_data(data)
16: eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
17: withCallingHandlers(code, warning = function(condition) {    if (ignore_deprecation && is_deprecation(condition)) {        return()    }    out$push(condition)    maybe_restart("muffleWarning")})
18: .capture(act$val <- eval_bare(quo_get_expr(.quo), quo_get_env(.quo)),     ...)
19: quasi_capture(enquo(object), label, capture_warnings, ignore_deprecation = identical(regexp,     NA))
20: expect_warning(x <- plot_data(data))
21: eval(code, test_env)
22: eval(code, test_env)
23: withCallingHandlers({    eval(code, test_env)    if (!handled && !is.null(test)) {        skip_empty()    }}, expectation = handle_expectation, skip = handle_skip, warning = handle_warning,     message = handle_message, error = handle_error)
24: doTryCatch(return(expr), name, parentenv, handler)
25: tryCatchOne(expr, names, parentenv, handlers[[1L]])
26: tryCatchList(expr, names[-nh], parentenv, handlers[-nh])
27: doTryCatch(return(expr), name, parentenv, handler)
28: tryCatchOne(tryCatchList(expr, names[-nh], parentenv, handlers[-nh]),     names[nh], parentenv, handlers[[nh]])
29: tryCatchList(expr, classes, parentenv, handlers)
30: tryCatch(withCallingHandlers({    eval(code, test_env)    if (!handled && !is.null(test)) {        skip_empty()    }}, expectation = handle_expectation, skip = handle_skip, warning = handle_warning,     message = handle_message, error = handle_error), error = handle_fatal,     skip = function(e) {    })
31: test_code(desc, code, env = parent.frame(), default_reporter = local_interactive_reporter())
32: test_that("plot_data", {    data <- data.frame(x = 1:10, y = rep(2, 10), y2 = c(1, 4,         6, 7, 9, 11, 2, 2, 1, 5), str = paste0("txt", 1:10),         factor = factor(10:1), stringsAsFactors = FALSE)    expect_warning(x <- plot_data(data))    expect_identical(length(x), 6L)    expect_true(all(vapply(x, inherits, TRUE, what = "ggplot")))})
33: eval(code, test_env)
34: eval(code, test_env)
35: withCallingHandlers({    eval(code, test_env)    if (!handled && !is.null(test)) {        skip_empty()    }}, expectation = handle_expectation, skip = handle_skip, warning = handle_warning,     message = handle_message, error = handle_error)
36: doTryCatch(return(expr), name, parentenv, handler)
37: tryCatchOne(expr, names, parentenv, handlers[[1L]])
38: tryCatchList(expr, names[-nh], parentenv, handlers[-nh])
39: doTryCatch(return(expr), name, parentenv, handler)
40: tryCatchOne(tryCatchList(expr, names[-nh], parentenv, handlers[-nh]),     names[nh], parentenv, handlers[[nh]])
41: tryCatchList(expr, classes, parentenv, handlers)
42: tryCatch(withCallingHandlers({    eval(code, test_env)    if (!handled && !is.null(test)) {        skip_empty()    }}, expectation = handle_expectation, skip = handle_skip, warning = handle_warning,     message = handle_message, error = handle_error), error = handle_fatal,     skip = function(e) {    })
Error: Error: R CMD check found ERRORs
Execution halted
43: test_code(test = NULL, code = exprs, env = env, default_reporter = StopReporter$new())
44: source_file(path, env = env(env), desc = desc, error_call = error_call)
45: FUN(X[[i]], ...)
46: lapply(test_paths, test_one_file, env = env, desc = desc, error_call = error_call)
47: doTryCatch(return(expr), name, parentenv, handler)
48: tryCatchOne(expr, names, parentenv, handlers[[1L]])
49: tryCatchList(expr, classes, parentenv, handlers)
50: tryCatch(code, testthat_abort_reporter = function(cnd) {    cat(conditionMessage(cnd), "\n")    NULL})
51: with_reporter(reporters$multi, lapply(test_paths, test_one_file,     env = env, desc = desc, error_call = error_call))
52: test_files_serial(test_dir = test_dir, test_package = test_package,     test_paths = test_paths, load_helpers = load_helpers, reporter = reporter,     env = env, stop_on_failure = stop_on_failure, stop_on_warning = stop_on_warning,     desc = desc, load_package = load_package, error_call = error_call)
53: test_files(test_dir = path, test_paths = test_paths, test_package = package,     reporter = reporter, load_helpers = load_helpers, env = env,     stop_on_failure = stop_on_failure, stop_on_warning = stop_on_warning,     load_package = load_package, parallel = parallel)
54: test_dir("testthat", package = package, reporter = reporter,     ..., load_package = "installed")
55: test_check("embr")
An irrecoverable exception occurred. R is aborting now ...

@joethorley
Copy link
Member

Note there is this related issue that Kirill filed for jmbr

poissonconsulting/jmbr#45

@dunkenwg dunkenwg assigned dunkenwg and joethorley and unassigned dunkenwg Jan 29, 2025
@dunkenwg
Copy link
Contributor Author

Fixed the macOS R-CMD-check by following this repo's action:

- name: Install JAGS (if macOS)
   if: runner.os == 'macOS'
      # This worked before R 4.3: brew install jags
      run: |
         curl -o wjags.pkg -L0 -k --url https://downloads.sourceforge.net/project/mcmc-jags/JAGS/4.x/Mac%20OS%20X/JAGS-4.3.2.pkg
         sudo installer -pkg wjags.pkg -target /
         rm wjags.pkg

brew install jags no longer works because Homebrew's library path has changed from /usr/local/lib/ to /opt/homebrew/. A .pkg installer works though!

@joethorley
Copy link
Member

Awesome - well done!

@joethorley
Copy link
Member

@dunkenwg - can you do the same fix for jmbr?

@dunkenwg
Copy link
Contributor Author

dunkenwg commented Feb 5, 2025

@aylapear, would you review this when you have a chance?

Copy link
Member

@aylapear aylapear left a comment

Choose a reason for hiding this comment

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

Awesome job on figuring that fix out!

@dunkenwg dunkenwg enabled auto-merge February 6, 2025 05:29
@dunkenwg dunkenwg disabled auto-merge February 6, 2025 05:30
@dunkenwg
Copy link
Contributor Author

dunkenwg commented Feb 6, 2025

@joethorley or @aylapear, how do I update the status of this pull request so that the rcc action will run and I can merge?

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.

3 participants