-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix cache publish wheels #103
Conversation
db892ce
to
03b54a8
Compare
- uses: actions/checkout@v3 | ||
with: | ||
fetch-depth: 0 | ||
|
||
- uses: actions/cache@v3 | ||
id: cache-package-check | ||
with: | ||
key: carolina_dist_${{ matrix.os }}_python-${{ matrix.python-version }}_boost-${{ inputs.BOOST_VERSION }}_dakota-${{ inputs.DAKOTA_VERSION }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this key (and its equivalent at L60) can be renamed to something indicating that is the deps
and not dist
it represents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this contains deps.
if: steps.cache-package-check.outputs.cache-hit != 'true' | ||
with: | ||
fetch-depth: 0 | ||
path: ./dist_build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this path, it is deps
and not dist
I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this contains deps.
make --debug=b -j8 install &> /github/workspace/trace/dakota_install.log | ||
|
||
|
||
DIST_BUILD=/github/workspace/dist_build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call it DEPS_BUILD
instead of dist, as it is the directory with the dependencies you are copying to the install dir (which is closer to being the dist dir AFAIK)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that, this makes sense. Fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactor, caching built boost & dakota but always rebuilding the wheels, only some smaller comments on how things are named, I think the naming should reflect that we (1) build deps
, (2) copy the deps to the install dir, in order to build the dist
stuff
Feedback from review
d2dc3d7
to
386a5c2
Compare
Resolves #95
Separates dependency-building from wheel generation such that we generate new wheels when releasing instead of using the cached ones.
This resolves the bug mentioned above, where caching prohibited the newly tagged release wheels being built.