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

ci: Enable conditional SAT solving #1177

Merged
merged 8 commits into from
Sep 2, 2024
Merged

ci: Enable conditional SAT solving #1177

merged 8 commits into from
Sep 2, 2024

Conversation

AlejandroCabeza
Copy link
Collaborator

@AlejandroCabeza AlejandroCabeza commented Aug 13, 2024

Use only on a daily job so we test out if it behaves correctly.

closes: #1174

@diegomrsantos
Copy link
Contributor

Please make sure the changes are working as expected.

@AlejandroCabeza
Copy link
Collaborator Author

Please make sure the changes are working as expected.

Verified.

@@ -86,4 +91,7 @@ jobs:
run: |
nim --version
nimble --version
NIMFLAGS="${NIMFLAGS} --mm:${{ matrix.nim.memory_management }}" nimble test
[[ ${{ inputs.use_sat_solver }} ]] && dependency_solver="sat" || dependency_solver="legacy"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[[ ${{ inputs.use_sat_solver }} ]] && dependency_solver="sat" || dependency_solver="legacy"
if [[ "${{ inputs.use_sat_solver }}" == "true" ]]; then
dependency_solver="sat"
else
dependency_solver="legacy"
fi

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any reason you oppose to ternary?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's more readable in this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

But just a suggestion, up to you

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, you're right; that ternary is a bit convoluted.

@@ -86,4 +91,7 @@ jobs:
run: |
nim --version
nimble --version
NIMFLAGS="${NIMFLAGS} --mm:${{ matrix.nim.memory_management }}" nimble test
[[ ${{ inputs.use_sat_solver }} ]] && dependency_solver="sat" || dependency_solver="legacy"
NIMFLAGS="${NIMFLAGS} --mm:${{ matrix.nim.memory_management }} --solver:$dependency_solver"
Copy link
Contributor

@diegomrsantos diegomrsantos Aug 30, 2024

Choose a reason for hiding this comment

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

Suggested change
NIMFLAGS="${NIMFLAGS} --mm:${{ matrix.nim.memory_management }} --solver:$dependency_solver"
NIMFLAGS="${NIMFLAGS} --mm:${{ matrix.nim.memory_management }} --solver:${dependency_solver}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is written as it was before: Instead of assuming NIMFLAGS is empty we just append the values to it, as it's common practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I updated it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's about the missing {}

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably better to change the name to upper case too.

Copy link
Collaborator Author

@AlejandroCabeza AlejandroCabeza Sep 2, 2024

Choose a reason for hiding this comment

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

Ah, I see, I misunderstood.
Right, it would work regardless as $variable can be used interchangeably with ${variable}; not the other way around, though.

EDIT: Updated it just because I like it more with brackets 😆

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd kinda keep the lowercase, though, as uppercase is used for env variables while lowercase for local ones. IIRC.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.51%. Comparing base (02c96fc) to head (c1d5f52).
Report is 48 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1177      +/-   ##
==========================================
- Coverage   84.53%   84.51%   -0.03%     
==========================================
  Files          91       92       +1     
  Lines       15517    16427     +910     
==========================================
+ Hits        13118    13884     +766     
- Misses       2399     2543     +144     

see 81 files with indirect coverage changes

Copy link
Contributor

@diegomrsantos diegomrsantos left a comment

Choose a reason for hiding this comment

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

Thank you

@AlejandroCabeza AlejandroCabeza merged commit 70754cd into master Sep 2, 2024
14 checks passed
@AlejandroCabeza AlejandroCabeza deleted the ci/sat-solver branch September 2, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

Add separate CI job using SAT solver
3 participants