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

CMake Chem and Chem+KPP Build #2018

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from
Open

Conversation

islas
Copy link
Collaborator

@islas islas commented Mar 11, 2024

TYPE: enhancement

KEYWORDS: cmake, chem, kpp

SOURCE: internal

DESCRIPTION OF CHANGES:
Problem:
Current CMake build does not build chem or chem+kpp configurations

Solution:
Build kpp and associated tools, and cmake commands to facilitate simplified logic of the configure_wkc and compile_wkc scripts. As with all CMake builds, all auto-generated source code is placed in the out-of-source build directory.

Notable differences to make build :

  • Use of Bison instead of Yacc as it is more easily accessible for install and usage as well as backward compatible
  • Allow -j N parallel jobs to generate KPP sources up to a limit
  • Use KPP-generated source file original names (not renamed to module_kpp_*
  • Pass tuv_kpp a directory to locate where include file is to be generated, and allow control of file IO mode*
  • Allow integration decomp rewrite to specify file locations rather than hard-coded*
  • registry uses -DWRF_CHEM and -DWRF_KPP defines passed at command line instead of getenv() to match all other options*

*Affects make build in subtle ways but do not change user instructions

LIST OF MODIFIED FILES:
M CMakeLists.txt
M chem/CMakeLists.txt
A chem/KPP/CMakeLists.txt
M chem/KPP/compile_wkc
A chem/KPP/kpp/kpp-2.1/CMakeLists.txt
A chem/KPP/util/wkc/CMakeLists.txt
M chem/KPP/util/wkc/gen_kpp.c
M chem/KPP/util/wkc/protos_kpp.h
M chem/KPP/util/wkc/tuv_kpp.c
A chem/KPP/util/write_decomp/CMakeLists.txt
M chem/KPP/util/write_decomp/Makefile
M chem/KPP/util/write_decomp/integr_edit.c
M chem/chem_driver.F
M tools/CMakeLists.txt
M tools/data.h
M tools/registry.c

TESTS CONDUCTED:

  1. Reproduction of chem and chem+kpp regtests with cmake is possible now

RELEASE NOTE:
CMake Chem and Chem+KPP Build

@islas islas requested review from a team as code owners March 11, 2024 20:51
@weiwangncar
Copy link
Collaborator

The Jenkins test results:

Test Type              | Expected  | Received |  Failed
= = = = = = = = = = = = = = = = = = = = = = = =  = = = =
Number of Tests        : 23           24
Number of Builds       : 60           57
Number of Simulations  : 158           150        0
Number of Comparisons  : 95           86        0

Failed Simulations are: 
None
Which comparisons are not bit-for-bit: 
None

@islas islas requested review from a team as code owners August 6, 2024 02:34
@islas islas changed the base branch from develop to release-v4.6.1 August 6, 2024 03:08
@islas
Copy link
Collaborator Author

islas commented Aug 6, 2024

Requires #2056, #2053, #2086, #2087, and #2088

@islas islas changed the base branch from release-v4.6.1 to develop December 9, 2024 23:38
@amstokely amstokely self-requested a review December 16, 2024 17:20
amstokely
amstokely previously approved these changes Dec 16, 2024
Copy link
Collaborator

@amstokely amstokely left a comment

Choose a reason for hiding this comment

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

While replacing the call to system(cp_command) with a safer alternative would be a good improvement, this system call was not introduced in this PR. Addressing it might be better suited for a future PR.


system(cp_command);
#ifndef NO_COPY
system(cp_command);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Calling system() with a command-line-provided argument introduces significant security risks, as it can allow command injection if the input is not carefully sanitized. Could we replace system(cp_command) with a safer alternative that avoids directly invoking the shell?

For example, the following implementation securely copies files using fork() and execlp():

#include <unistd.h>
#include <sys/wait.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

void copy_file(const char *source, const char *destination) {
    pid_t pid = fork();
    if (pid == 0) {
        // Child process: Securely execute the `cp` command
        execlp("cp", "cp", source, destination, NULL);
        perror("execlp failed"); // If exec fails
        exit(EXIT_FAILURE);
    } else if (pid > 0) {
        // Parent process: Wait for the child to complete
        int status;
        waitpid(pid, &status, 0);
        if (WIFEXITED(status)) {
            printf("Copy completed with exit code %d\n", WEXITSTATUS(status));
        } else {
            fprintf(stderr, "Copy process terminated abnormally\n");
        }
    } else {
        perror("fork failed");
        exit(EXIT_FAILURE);
    }
}

int main(int argc, char *argv[]) {
    if (argc != 2) {
        fprintf(stderr, "Usage: %s \"<source_file> <destination_file>\"\n", argv[0]);
        return EXIT_FAILURE;
    }

    // Parse the cp_command argument into source and destination
    char *cp_command = argv[1];
    char *source = strtok(cp_command, " ");
    char *destination = strtok(NULL, " ");

    if (!source || !destination) {
        fprintf(stderr, "Error: Invalid command. Provide source and destination files.\n");
        return EXIT_FAILURE;
    }

    // Call the copy_file function
    printf("Copying '%s' to '%s'...\n", source, destination);
    copy_file(source, destination);

    return EXIT_SUCCESS;
}

mgduda
mgduda previously approved these changes Dec 20, 2024
Copy link
Collaborator

@mgduda mgduda left a comment

Choose a reason for hiding this comment

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

Everything seems to compile successfully if I select chem and kpp as additional configuration options, though I actually have no idea how to run anything with chem+kpp to verify that the compiled model includes chemistry functionality.

@islas islas dismissed stale reviews from mgduda and amstokely via 7adbe6e December 21, 2024 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants