Skip to content

ci(conventions): exempt cmake/SystemDeps.cmake from the system-dependency rule - #2373

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
011010:ci/conventions-systemdeps-exemption
Oct 11, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
011010:ci/conventions-systemdeps-exemption

Conversation

@011010

@011010 011010 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

What

Exempts cmake/SystemDeps.cmake from the system-dependency conventions rule, so the opt-in system resolution added by #921 can pass the Conventions check.

The exemption has to live in main: conventions.yml checks out the base SHA and runs tools/check_conventions.py from there, so the copy added inside #921 is ignored until this lands.

Tested

  • python3 -I tools/check_conventions.py --base main --head ci/conventions-systemdeps-exemption on this branch: All conventions checks passed.
  • Running the same checker over build: add ANYPS5_USE_SYSTEM_DEPS option #921's head (pr921-rebased) against main: the system-dependency findings in cmake/SystemDeps.cmake drop from 6 to 0, with no new finding in any other file.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes

@011010 011010 mentioned this pull request Oct 9, 2026
9 tasks done
@011010

011010 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

This is the prerequisite for #921: the Conventions job runs the trusted base script, so #921's own exemption is ignored until this lands in main. Could someone review/merge it so I can rebase #921 and drop the duplicate hunk there? @adriadam10

@adriadam10

Copy link
Copy Markdown
Collaborator

The change itself is fine: the exemption is a path match in tools/check_conventions.py, the checker passes on this head, and it is the only way #921's file can clear the base-script check.

But it carves a path out of a CONTRIBUTING rule ("Third-party code is added as a submodule under 3rdparty/ and built from source, not found on the system"), and it only makes sense if opt-in system dependencies (#921) are accepted. That is a policy call.

@boykopovar @oneandonlydean: do you want an opt-in ANYPS5_USE_SYSTEM_DEPS path at all? If yes, this exemption (and a CONTRIBUTING line saying where system resolution is allowed) can go in; if no, this and #921 should be closed. Since it changes what CI checks, the full Build should be dispatched on it before merging.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

The owner already said yes to an opt-in ANYPS5_USE_SYSTEM_DEPS on #400 (#400 (comment)), as long as it is off by default, fails at configure time when a package is missing, and never falls back to building from source. So this exemption can go in; whether #921 meets those three is for #921's review.

@Mouhand-Kaddo

Copy link
Copy Markdown
Contributor

Additional platform validation on standalone PR head 1c6e53b9bf454346c9d01b195549f0de37dfaaa8:

Windows: passed — Windows 11, supported WinLibs GCC 15.2.0 UCRT/POSIX/SEH r7; CMake 4.3.1 / Ninja.

Ran the real conventions checker against the system-dependency diff from #921, comparing the checker before and after this PR. The six intended find_package findings disappeared and no new findings appeared. This is a focused behavior check, not a full runtime build.

@adriadam10

Copy link
Copy Markdown
Collaborator

The owner accepted the opt-in ANYPS5_USE_SYSTEM_DEPS on #400 (off by default, fails at configure time, no fallback), so this exemption is settled. Checked: main + this PR's checker passes on #921's rebased head (main's fails it with 6 system-dependency errors), and CI is green here; merging so #921 can follow.

@adriadam10
adriadam10 merged commit 407b532 into boykopovar:main Oct 11, 2026
7 checks passed
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.

4 participants