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

Minion: Use minion's build system not build.rs to build libminion, and CI tweaks #5

Merged
merged 6 commits into from
Sep 29, 2023

Conversation

niklasdewally
Copy link
Contributor

@niklasdewally niklasdewally commented Sep 28, 2023

#1

Chris Jefferson has added support for building Minion as a library through its own build system: minion/minion#6.

This PR removes the code in build.rs that builds the libminion.a library, and instead calls minion's build system to do this for us.


I have also implemented CI caching as per #4.

The primary issue was the time needed to build minion. Therefore, minion's build directory is cached so that make can do proper incremental builds.

Also, build.rs should now not run at all unless it or vendor code changes.

@niklasdewally
Copy link
Contributor Author

Screenshot_2023-09-28-18-40-44-48_40deb401b9ffe8e1df2f1cc5ba480b12.jpg

.github/workflows/minion-tests.yml Outdated Show resolved Hide resolved
.github/workflows/minion-tests.yml Outdated Show resolved Hide resolved
.gitmodules Show resolved Hide resolved
@ozgurakgun
Copy link
Contributor

I think this works for minion caching, yes! I was thinking it would be easier to give hashFiles more files.

Oh also, for restore-keys I think you can just say ${{ runner.os }}. It picks the newest matching cache anyway.

Merging like this, we can always improve it later & if needed.

@niklasdewally
Copy link
Contributor Author

niklasdewally commented Sep 28, 2023

Oh also, for restore-keys I think you can just say ${{ runner.os }}. It picks the newest matching cache anyway.

When minion updates (especially when new files are added), a clean build is sometimes needed for it to work properly. Requiring an exact match for minion in the cache would force a clean build in CI when needed.

(build.rs does not re-run ./configure for minion if the build directory already exists, as ./configure effectively forces a clean build.)

@niklasdewally
Copy link
Contributor Author

@ozgurakgun is this ready for merge?

@ozgurakgun ozgurakgun merged commit 54c3873 into conjure-cp:main Sep 29, 2023
3 checks passed
@niklasdewally niklasdewally deleted the build-using-minion branch September 29, 2023 13:13
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.

2 participants