diff --git a/.Rbuildignore b/.Rbuildignore index 9a84d26..df20826 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -3,6 +3,7 @@ ^\.covrignore$ ^\.github$ ^.*\.Rproj$ +^paper$ ^\_pkgdown.yml$ .clang-format README.md diff --git a/DESCRIPTION b/DESCRIPTION index 56d9eed..6133daa 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -15,11 +15,11 @@ Imports: methods, tools URL: https://CRAN.R-project.org/package=MCSimMod, https://github.com/USEPA/MCSimMod -License: GPL-3 +License: GPL-3 + file LICENSE Encoding: UTF-8 LazyData: true Roxygen: list(markdown = TRUE) -RoxygenNote: 7.3.2 +RoxygenNote: 7.3.3 Suggests: knitr, rmarkdown, diff --git a/R/MCSim_model.R b/R/MCSim_model.R index dca67ff..8b6a01f 100644 --- a/R/MCSim_model.R +++ b/R/MCSim_model.R @@ -33,9 +33,10 @@ Model <- setRefClass("Model", #' @field paths List of character strings that are names of files associated with the model. #' @field writeTemp Boolean specifying whether to write model files to a temporary directory. If value is TRUE, model files will be written to a temporary directory; if value is FALSE, model files will be written to the same directory that contains the model specification file. #' @field verboseOutput Boolean specifying whether to write translator messages to standard output. If value is TRUE, messages will be written to standard output; if value is FALSE, messages will be written to files in a temporary directory. + #' @field recompiled Boolean specifying if the model has been recompiled due to change in source file mName = "character", mString = "character", initParms = "function", initStates = "function", Outputs = "ANY", parms = "numeric", Y0 = "numeric", - paths = "list", writeTemp = "logical", verboseOutput = "logical" + paths = "list", writeTemp = "logical", verboseOutput = "logical", recompiled = "logical" ), methods = list( initialize = function(...) { @@ -53,20 +54,25 @@ Model <- setRefClass("Model", } file <- tempfile(pattern = "mcsimmod_", fileext = ".model") writeLines(mString, file) + source_file <- normalizePath(file, winslash = "/") + file <- source_file } else { if (writeTemp == TRUE) { - source_file <- normalizePath(paste0(mName, ".model")) + source_file <- normalizePath(paste0(mName, ".model"), winslash = "/") temp_directory <- tempdir() file <- file.path(temp_directory, basename(source_file)) - file_copied <- file.copy(from = source_file, to = file) + file.copy(from = source_file, to = file) + file <- normalizePath(file, winslash = "/") } else { - file <- normalizePath(paste0(mName, ".model")) + source_file <- normalizePath(paste0(mName, ".model"), winslash = "/") + file <- source_file } } mList <- .fixPath(file) + sList <- .fixPath(source_file) mName <<- mList$mName mPath <- mList$mPath - + sPath <- sList$mPath paths <<- list( dll_name = paste0(mName, "_model"), @@ -75,17 +81,26 @@ Model <- setRefClass("Model", dll_file = file.path(mPath, paste0(mName, "_model", .Platform$dynlib.ext)), inits_file = file.path(mPath, paste0(mName, "_model_inits.R")), model_file = file.path(mPath, paste0(mName, ".model")), + source_file = file.path(sPath, paste0(mName, ".model")), hash_file = file.path(mPath, paste0(mName, "_model.md5")) ) + + # Calculate and save initial hash during initialization + # This allows loadModel to immediately check if source has changed + if (!file.exists(paths$hash_file)) { + initial_hash <- as.character(tools::md5sum(paths$source_file)) + write(initial_hash, file = paths$hash_file) + } }, loadModel = function(force = FALSE) { "Translate (if necessary) the model specification text to C, compile (if necessary) the resulting C file to create a dynamic link library (DLL) file (on Windows) or a shared object (SO) file (on Unix), and then load all essential information about the Model object into memory (for use in the current R session)." hash_exists <- file.exists(paths$hash_file) if (hash_exists) { - hash_has_changed <- .fileHasChanged(paths$model_file, paths$hash_file) + hash_has_changed <- .fileHasChanged(paths$source_file, paths$hash_file) } else { hash_has_changed <- TRUE } + recompiled <<- FALSE # Conditions for compiling a model: # 1. The DLL (on Windows) or SO (on Unix) associated with the model @@ -98,7 +113,12 @@ Model <- setRefClass("Model", # specification file has been changed since the last translation and # compiling. if (!file.exists(paths$dll_file) | (force) | (!hash_exists) | (hash_exists & hash_has_changed)) { - compileModel(paths$model_file, paths$c_file, paths$dll_name, paths$dll_file, hash_file = paths$hash_file, verbose_output = verboseOutput) + # When using writeTemp=TRUE, ensure the model file is updated from source before compilation + if (writeTemp & (paths$source_file != paths$model_file)) { + file.copy(from = paths$source_file, to = paths$model_file, overwrite = TRUE) + } + compileModel(paths$model_file, paths$c_file, paths$dll_name, paths$dll_file, source_file = paths$source_file, hash_file = paths$hash_file, verbose_output = verboseOutput) + recompiled <<- TRUE } # Load the compiled model (DLL). diff --git a/R/compileModel.R b/R/compileModel.R index cef24fd..c4625d2 100644 --- a/R/compileModel.R +++ b/R/compileModel.R @@ -8,13 +8,14 @@ #' @param c_file Name of a C source code file to be created by compiling the MCSim model specification file. #' @param dll_name Name of a DLL or SO file without the extension (".dll" or ".so"). #' @param dll_file Name of the same DLL or SO file with the appropriate extension (".dll" or ".so"). -#' @param hash_file Name of a file containing a hash key for determining if `model_file` has changed since the previous translation and compilation. +#' @param source_file Name of the original source file to use for hash calculation. Defaults to \code{model_file} for backward compatibility. When \code{writeTemp=TRUE} in \code{createModel()}, this should be set to the original source file path to ensure hash tracking works correctly when the source file is separate from the compiled model file. +#' @param hash_file Name of a file containing a hash key for determining if `source_file` has changed since the previous translation and compilation. #' @param verbose_output Boolean specifying whether to write translator messages to standard output. If value is TRUE, messages will be written to standard output; if value is FALSE, messages will be written to files in a temporary directory. #' @returns No return value. Creates files and saves them in locations specified by function arguments. #' @import tools #' @useDynLib MCSimMod, .registration=TRUE #' @export -compileModel <- function(model_file, c_file, dll_name, dll_file, hash_file = NULL, verbose_output = FALSE) { +compileModel <- function(model_file, c_file, dll_name, dll_file, source_file = model_file, hash_file = NULL, verbose_output = FALSE) { # Unload DLL if it has been loaded. mList <- .fixPath(model_file) model_name <- mList$mName @@ -120,7 +121,7 @@ compileModel <- function(model_file, c_file, dll_name, dll_file, hash_file = NUL "event", "root" ) - for (idx in seq(length(item_to_replace))) { + for (idx in seq_along(item_to_replace)) { lines <- gsub( paste0("\\b", item_to_replace[idx], "\\b"), paste0(item_to_replace[idx], "_", model_name), @@ -151,7 +152,7 @@ compileModel <- function(model_file, c_file, dll_name, dll_file, hash_file = NUL "initStates", "initState" ) - for (idx in seq(length(item_to_replace))) { + for (idx in seq_along(item_to_replace)) { lines <- gsub( paste0("\\b", item_to_replace[idx], "\\b"), paste0(item_to_replace[idx], "_", model_name), @@ -181,10 +182,10 @@ compileModel <- function(model_file, c_file, dll_name, dll_file, hash_file = NUL normalizePath(out_file), ".\n" ) - # If hash file name was provided, create a hash (md5 sum) for the model file + # If hash file name was provided, create a hash (md5 sum) for the source file # and print a message about its location. if (!is.null(hash_file)) { - file_hash <- as.character(md5sum(model_file)) + file_hash <- as.character(tools::md5sum(source_file)) write(file_hash, file = hash_file) message( "Hash created and saved in the file ", normalizePath(hash_file), diff --git a/R/fileHasChanged.R b/R/fileHasChanged.R index fe93f0a..bf9ac9c 100644 --- a/R/fileHasChanged.R +++ b/R/fileHasChanged.R @@ -1,11 +1,11 @@ #----------------- # compareHash #---------------- -# Private function to determine if the .model file has changed +# Private function to determine if the source .model file has changed -.fileHasChanged <- function(model_file, hash_file) { +.fileHasChanged <- function(source_file, hash_file) { # Calculate hash for current model file - current_hash <- as.character(md5sum(model_file)) + current_hash <- tools::md5sum(source_file) # Read saved hash saved_hash <- readLines(hash_file, n = 1) diff --git a/R/fixPath.R b/R/fixPath.R index d2b1998..d7a5b47 100644 --- a/R/fixPath.R +++ b/R/fixPath.R @@ -7,9 +7,6 @@ .fixPath <- function(file) { new.mName <- strsplit(basename(file), "[.]")[[1]][1] new.mPath <- dirname(file) - if (.Platform$OS.type == "windows") { - new.mPath <- gsub("\\\\", "/", utils::shortPathName(new.mPath)) - } has_space <- grepl(" ", new.mPath) if (has_space == T) { diff --git a/man/Model-class.Rd b/man/Model-class.Rd index 1925d91..cb31ee2 100644 --- a/man/Model-class.Rd +++ b/man/Model-class.Rd @@ -50,6 +50,8 @@ expression \code{mod$updateParms()}. Use the \code{createModel()} function to cr \item{\code{writeTemp}}{Boolean specifying whether to write model files to a temporary directory. If value is TRUE, model files will be written to a temporary directory; if value is FALSE, model files will be written to the same directory that contains the model specification file.} \item{\code{verboseOutput}}{Boolean specifying whether to write translator messages to standard output. If value is TRUE, messages will be written to standard output; if value is FALSE, messages will be written to files in a temporary directory.} + +\item{\code{recompiled}}{Boolean specifying if the model has been recompiled due to change in source file} }} \section{Methods}{ diff --git a/man/compileModel.Rd b/man/compileModel.Rd index f5f7314..7739aac 100644 --- a/man/compileModel.Rd +++ b/man/compileModel.Rd @@ -9,6 +9,7 @@ compileModel( c_file, dll_name, dll_file, + source_file = model_file, hash_file = NULL, verbose_output = FALSE ) @@ -22,7 +23,9 @@ compileModel( \item{dll_file}{Name of the same DLL or SO file with the appropriate extension (".dll" or ".so").} -\item{hash_file}{Name of a file containing a hash key for determining if \code{model_file} has changed since the previous translation and compilation.} +\item{source_file}{Name of the original source file to use for hash calculation. Defaults to \code{model_file} for backward compatibility. When \code{writeTemp=TRUE} in \code{createModel()}, this should be set to the original source file path to ensure hash tracking works correctly when the source file is separate from the compiled model file.} + +\item{hash_file}{Name of a file containing a hash key for determining if \code{source_file} has changed since the previous translation and compilation.} \item{verbose_output}{Boolean specifying whether to write translator messages to standard output. If value is TRUE, messages will be written to standard output; if value is FALSE, messages will be written to files in a temporary directory.} } diff --git a/tests/testthat/test-compareHash.R b/tests/testthat/test-compareHash.R index 84f5833..09af5b0 100644 --- a/tests/testthat/test-compareHash.R +++ b/tests/testthat/test-compareHash.R @@ -22,3 +22,50 @@ testthat::test_that("test_compareHash", { model$cleanup() }) + +testthat::test_that("test_tmp_compileModel", { + # Test writeTemp=TRUE functionality with proper source/compilation file separation + # Create a proper source file in a permanent location (not temp) + source_dir <- file.path(tempdir(), "sourceDir") + dir.create(source_dir, showWarnings = FALSE) + mName <- file.path(source_dir, "test_model") + mString <- readLines(file.path(testthat::test_path(), "data", "exponential.model")) + writeLines(mString, paste0(mName, ".model")) + + # Verify source file exists + testthat::expect_true(file.exists(paste0(mName, ".model"))) + + # Create model with writeTemp=TRUE - this should use the source file + # and create compilation files in temp directory + model <- createModel(mName, writeTemp = TRUE) + + # Verify paths are properly separated + testthat::expect_true(model$paths$source_file != model$paths$model_file) + testthat::expect_true(file.exists(model$paths$source_file)) + testthat::expect_true(file.exists(model$paths$model_file)) + + # First load - should compile + model$loadModel() + testthat::expect_true(file.exists(model$paths$hash_file)) # Check if hash was created + testthat::expect_true(model$recompiled) # Should be TRUE on first compile + + # Second load - should NOT recompile (no changes) + model$loadModel() + testthat::expect_false(.fileHasChanged(model$paths$source_file, model$paths$hash_file)) + testthat::expect_false(model$recompiled) # Should be FALSE, no recompile needed + + # Edit source file (what users actually edit) + write("# File is edited", file = model$paths$source_file, append = TRUE, sep = "\n") + testthat::expect_true(.fileHasChanged(model$paths$source_file, model$paths$hash_file)) + + # Third load after edit - should recompile + model$loadModel() + testthat::expect_true(model$recompiled) # Should be TRUE, source changed + + # Fourth load - should NOT recompile (our bug fix test!) + model$loadModel() + testthat::expect_false(.fileHasChanged(model$paths$source_file, model$paths$hash_file)) + testthat::expect_false(model$recompiled) # Should be FALSE, hash should now match + + model$cleanup() +}) diff --git a/tests/testthat/test-sim.R b/tests/testthat/test-sim.R index e2549bd..11bffba 100644 --- a/tests/testthat/test-sim.R +++ b/tests/testthat/test-sim.R @@ -45,7 +45,7 @@ testthat::test_that("Model$absoluteModel", { # Use absolute path of temp directory, # Test to make sure changing the file returns a changed path - dir.create(file.path(tempdir(), "testDir")) + dir.create(file.path(tempdir(), "testDir"), showWarnings = FALSE) mName <- tempfile(pattern = "mcsimmod_", tmpdir = file.path(tempdir(), "testDir")) mString <- readLines(file.path(testthat::test_path(), "data", "exponential.model")) writeLines(mString, paste0(mName, ".model"))