add CoolProp development testing - #52
Conversation
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a CoolPropDevLoader test package that downloads and compiles a dev CoolProp library from SourceForge, switches ChangesDev library loader and preference wiring
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/CoolProp.jl (1)
9-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider combining the two
@static if VERSION >= v"1.6"blocks.Having separate
@staticblocks forusing Preferencesandconst libcoolpropduplication of the version guard. A single block is clearer and ensures the import and constant are always consistent.♻️ Proposed refactor
`@static` if VERSION >= v"1.6" - using Preferences -end - -@static if VERSION >= v"1.6" const libcoolprop = load_preference(CoolProp_jll,"coolprop_library", CoolProp_jll.libcoolprop) else const libcoolprop = CoolProp_jll.libcoolprop end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CoolProp.jl` around lines 9 - 17, Combine the duplicated VERSION guard in CoolProp.jl by using a single `@static` if VERSION >= v"1.6" block around both the Preferences import and the libcoolprop assignment, and keep the fallback branch for older versions in the same structure. Update the top-level initialization near using Preferences and const libcoolprop so the import and preference-based library selection always stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test_dev.yml:
- Around line 1-58: The workflow needs tighter GitHub Actions security settings:
add an explicit minimal permissions block at the workflow level, since it only
runs tests, and disable token persistence in the Checkout step. Update the
`test-coolprop-dev` workflow by setting least-privilege permissions and changing
the `actions/checkout` usage so `persist-credentials` is false. Use the existing
`Checkout` step and the workflow header near `name: Development Tests` to locate
the changes.
In `@test/CoolPropDevLoader/Project.toml`:
- Line 12: The Julia compatibility bound is too low for the dependencies used by
CoolPropDevLoader. Update the compat entry in Project.toml from julia = "1.3" to
a minimum version that supports the unconditional imports in
CoolPropDevLoader.jl, namely Preferences and Downloads, so the project cannot
resolve on unsupported Julia versions. Keep the change scoped to the
Project.toml for the test/CoolPropDevLoader package and ensure the compat
matches the requirements of the CoolPropDevLoader module.
In `@test/CoolPropDevLoader/src/CoolPropDevLoader.jl`:
- Line 28: The extraction step in CoolPropDevLoader should not block on reruns
when files already exist. Update the `run` invocation that uses `unzip` so it
overwrites existing files unconditionally, and keep the logic tied to the
existing `mkpath`-created `build_dir` flow in `CoolPropDevLoader.jl`. This
change should be made in the same place as the current `run(\`unzip ...\`)` call
so CI and non-interactive runs don’t hang.
- Around line 95-104: The two `use_dev_library` overloads are writing
`coolprop_library` with different preference scopes, so align them to use the
same `set_preferences!` kwargs. Update
`use_dev_library(::CompileCoolPropFromSourceforge)` to match the
`use_dev_library(lib_src::String)` behavior (or vice versa) so `CoolProp_jll`
stores the preference consistently for the project that `CoolProp.jl` later
reads via `load_preference`. Keep the change localized to these
`use_dev_library` methods and preserve the existing `@info` logging.
---
Nitpick comments:
In `@src/CoolProp.jl`:
- Around line 9-17: Combine the duplicated VERSION guard in CoolProp.jl by using
a single `@static` if VERSION >= v"1.6" block around both the Preferences import
and the libcoolprop assignment, and keep the fallback branch for older versions
in the same structure. Update the top-level initialization near using
Preferences and const libcoolprop so the import and preference-based library
selection always stay consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 652479ad-5c83-4d7a-91d7-76528f6eedec
📒 Files selected for processing (7)
.github/workflows/test_dev.ymlProject.tomlREADME.mdsrc/CoolProp.jltest/CoolProp.jltest/CoolPropDevLoader/Project.tomltest/CoolPropDevLoader/src/CoolPropDevLoader.jl
| name: Development Tests | ||
|
|
||
| on: | ||
| schedule: | ||
| - cron: 0 0 * * * | ||
| workflow_dispatch: | ||
| push: | ||
| paths: | ||
| - '.github/**' | ||
| - 'test/CoolPropDevLoader/**' | ||
|
|
||
| concurrency: | ||
| # Skip intermediate builds: all builds except for builds on the `master` branch | ||
| # Cancel intermediate builds: only pull request builds | ||
| group: ${{ github.workflow }}-${{ github.ref }}-${{ github.ref != 'refs/heads/master' || github.run_number }} | ||
| cancel-in-progress: ${{ startsWith(github.ref, 'refs/pull/') }} | ||
|
|
||
| jobs: | ||
| test-coolprop-dev: | ||
| timeout-minutes: 30 | ||
| strategy: | ||
| matrix: | ||
| julia_version: | ||
| - '1.12' | ||
| os: | ||
| - ubuntu-latest | ||
| julia_arch: | ||
| - x64 | ||
| fail-fast: false | ||
| runs-on: ${{ matrix.os }} | ||
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v7 | ||
|
|
||
| - uses: julia-actions/setup-julia@v3 | ||
| with: | ||
| arch: ${{ matrix.julia_arch }} | ||
| version: ${{ matrix.julia_version }} | ||
| - uses: julia-actions/cache@v3 | ||
|
|
||
| - name: add CoolPropDevLoader | ||
| shell: julia --color=yes --project=. {0} | ||
| run: | | ||
| using Pkg | ||
| Pkg.develop(path="test/CoolPropDevLoader") | ||
|
|
||
| - name: Compiling CoolProp | ||
| shell: julia --color=yes --compiled-modules=no --project=test {0} | ||
| run: | | ||
| using Pkg | ||
| Pkg.develop(path="test/CoolPropDevLoader") | ||
| using CoolPropDevLoader | ||
| CoolPropDevLoader.use_dev_library(CoolPropDevLoader.CompileCoolPropFromSourceforge()) | ||
| Pkg.add("Test") | ||
|
|
||
| - uses: julia-actions/julia-runtest@v1 | ||
|
No newline at end of file |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add permissions block and set persist-credentials: false on checkout.
The workflow lacks a permissions block, defaulting to the token's full permissions set. Since this workflow only runs tests, it should use minimal permissions. Additionally, actions/checkout persists the GitHub token by default, which is unnecessary here and flagged by zizmor as a credential persistence risk.
🔒 Proposed security hardening
name: Development Tests
on:
schedule:
- cron: 0 0 * * *
workflow_dispatch:
push:
paths:
- '.github/**'
- 'test/CoolPropDevLoader/**'
+permissions:
+ contents: read
+
concurrency:
# Skip intermediate builds: all builds except for builds on the `master` branch
# Cancel intermediate builds: only pull request builds
group: ${{ github.workflow }}-${{ github.ref }}-${{ github.ref != 'refs/heads/master' || github.run_number }}
cancel-in-progress: ${{ startsWith(github.ref, 'refs/pull/') }}
jobs:
test-coolprop-dev:
timeout-minutes: 30
strategy:
matrix:
julia_version:
- '1.12'
os:
- ubuntu-latest
julia_arch:
- x64
fail-fast: false
runs-on: ${{ matrix.os }}
steps:
- name: Checkout
uses: actions/checkout@v7
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: Development Tests | |
| on: | |
| schedule: | |
| - cron: 0 0 * * * | |
| workflow_dispatch: | |
| push: | |
| paths: | |
| - '.github/**' | |
| - 'test/CoolPropDevLoader/**' | |
| concurrency: | |
| # Skip intermediate builds: all builds except for builds on the `master` branch | |
| # Cancel intermediate builds: only pull request builds | |
| group: ${{ github.workflow }}-${{ github.ref }}-${{ github.ref != 'refs/heads/master' || github.run_number }} | |
| cancel-in-progress: ${{ startsWith(github.ref, 'refs/pull/') }} | |
| jobs: | |
| test-coolprop-dev: | |
| timeout-minutes: 30 | |
| strategy: | |
| matrix: | |
| julia_version: | |
| - '1.12' | |
| os: | |
| - ubuntu-latest | |
| julia_arch: | |
| - x64 | |
| fail-fast: false | |
| runs-on: ${{ matrix.os }} | |
| steps: | |
| - name: Checkout | |
| uses: actions/checkout@v7 | |
| - uses: julia-actions/setup-julia@v3 | |
| with: | |
| arch: ${{ matrix.julia_arch }} | |
| version: ${{ matrix.julia_version }} | |
| - uses: julia-actions/cache@v3 | |
| - name: add CoolPropDevLoader | |
| shell: julia --color=yes --project=. {0} | |
| run: | | |
| using Pkg | |
| Pkg.develop(path="test/CoolPropDevLoader") | |
| - name: Compiling CoolProp | |
| shell: julia --color=yes --compiled-modules=no --project=test {0} | |
| run: | | |
| using Pkg | |
| Pkg.develop(path="test/CoolPropDevLoader") | |
| using CoolPropDevLoader | |
| CoolPropDevLoader.use_dev_library(CoolPropDevLoader.CompileCoolPropFromSourceforge()) | |
| Pkg.add("Test") | |
| - uses: julia-actions/julia-runtest@v1 | |
| name: Development Tests | |
| on: | |
| schedule: | |
| - cron: 0 0 * * * | |
| workflow_dispatch: | |
| push: | |
| paths: | |
| - '.github/**' | |
| - 'test/CoolPropDevLoader/**' | |
| permissions: | |
| contents: read | |
| concurrency: | |
| # Skip intermediate builds: all builds except for builds on the `master` branch | |
| # Cancel intermediate builds: only pull request builds | |
| group: ${{ github.workflow }}-${{ github.ref }}-${{ github.ref != 'refs/heads/master' || github.run_number }} | |
| cancel-in-progress: ${{ startsWith(github.ref, 'refs/pull/') }} | |
| jobs: | |
| test-coolprop-dev: | |
| timeout-minutes: 30 | |
| strategy: | |
| matrix: | |
| julia_version: | |
| - '1.12' | |
| os: | |
| - ubuntu-latest | |
| julia_arch: | |
| - x64 | |
| fail-fast: false | |
| runs-on: ${{ matrix.os }} | |
| steps: | |
| - name: Checkout | |
| uses: actions/checkout@v7 | |
| with: | |
| persist-credentials: false | |
| - uses: julia-actions/setup-julia@v3 | |
| with: | |
| arch: ${{ matrix.julia_arch }} | |
| version: ${{ matrix.julia_version }} | |
| - uses: julia-actions/cache@v3 | |
| - name: add CoolPropDevLoader | |
| shell: julia --color=yes --project=. {0} | |
| run: | | |
| using Pkg | |
| Pkg.develop(path="test/CoolPropDevLoader") | |
| - name: Compiling CoolProp | |
| shell: julia --color=yes --compiled-modules=no --project=test {0} | |
| run: | | |
| using Pkg | |
| Pkg.develop(path="test/CoolPropDevLoader") | |
| using CoolPropDevLoader | |
| CoolPropDevLoader.use_dev_library(CoolPropDevLoader.CompileCoolPropFromSourceforge()) | |
| Pkg.add("Test") | |
| - uses: julia-actions/julia-runtest@v1 |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 33-34: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 19-58: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test_dev.yml around lines 1 - 58, The workflow needs
tighter GitHub Actions security settings: add an explicit minimal permissions
block at the workflow level, since it only runs tests, and disable token
persistence in the Checkout step. Update the `test-coolprop-dev` workflow by
setting least-privilege permissions and changing the `actions/checkout` usage so
`persist-credentials` is false. Use the existing `Checkout` step and the
workflow header near `name: Development Tests` to locate the changes.
Source: Linters/SAST tools
There was a problem hiding this comment.
The code compiles CoolProp and creates a directory inside the test folder, only read permissions are not enough
There was a problem hiding this comment.
@longemen3000, good question — but this is a common mix-up between two different types of "permissions."
The permissions: block in a GitHub Actions workflow only controls the scope of the GITHUB_TOKEN used for calls to the GitHub API (e.g., pushing commits, creating releases/comments, writing to packages, etc.). It has no effect on local filesystem operations on the runner's disk.
Since this workflow's build step (compiling CoolProp via CMake, creating a build directory under test/, downloading the SourceForge archive, etc.) only writes to the runner's local disk and never pushes back to the repository or calls the GitHub API to write content, contents: read is sufficient. The job can still freely create directories, compile code, and download files locally — that's unrelated to the token's API scope.
So contents: read won't block the compile step; it just prevents the GITHUB_TOKEN from being used to push/write to the repo via the API, which this workflow doesn't do anyway. Let me know if you're hitting an actual permission-denied error from a specific step, and I can help dig into that separately.
✏️ Learnings added
Learnt from: longemen3000
Repo: CoolProp/CoolProp.jl PR: 52
File: .github/workflows/test_dev.yml:1-58
Timestamp: 2026-07-09T06:28:06.309Z
Learning: In CoolProp.jl's `.github/workflows/*.yml` files, the workflow author (longemen3000) may confuse the GitHub Actions `permissions:` block (which scopes the GITHUB_TOKEN for GitHub API calls) with local filesystem write permissions on the runner. Clarify that `permissions: contents: read` does not restrict local disk operations like compiling code or creating directories in `test/CoolPropDevLoader`, since those don't use the GITHUB_TOKEN.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
This PR implements:
libcoolpropconstant to the new compiled library (via Preferences.jl).At the moment, the github action runs, and fails (because the fix for CoolProp/CoolProp#3258 is still not merged)
The compiler code was done with assistance of an LLM, feel free to change it to something better.
CC @ibell : At the moment, the yml is configured to run in CoolProp.jl, but you could run it in CoolProp instead
Summary by CodeRabbit
HAPropsSIexample.