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

chore: add devcontainer setup #70

Merged
merged 23 commits into from
Dec 26, 2024
Merged

Conversation

KnorpelSenf
Copy link
Contributor

@KnorpelSenf KnorpelSenf commented Dec 13, 2024

This add a devcontainer setup. It means that any contributor no longer has to install various solvers before being able to work on this repo. In addition, it can serve as a reference for the required system setup when somebody wants to use different solvers.

This extracts the system setup into a new build script which is used both to test CI and to create the container.

You can test the setup by opening the project inside the dev container and then building the crate.

Please let me know if something is not working out of the box.

@lovasoa
Copy link
Collaborator

lovasoa commented Dec 13, 2024

Can we install the same packages as in https://github.com/rust-or/good_lp/blob/main/.github/workflows/rust.yml, and run the tests inside it ?

I fear that if the dev container is not tested, it will just silently break at some point in the future...

@KnorpelSenf
Copy link
Contributor Author

I don't see a way to directly install GitHub Actions into a Docker container. Without the actions, there is no conda, and without conda, there is no scip, and without scip, we cannot run the tests.

Instead, what we could do is to duplicate the logic of the actions into a setup script that gives us a reproducible system setup. This script can then be run both in CI and during container setup. Would that be acceptable?

@KnorpelSenf
Copy link
Contributor Author

This would actually have the advantage that all missing deps would be installed in the dev container

@lovasoa
Copy link
Collaborator

lovasoa commented Dec 13, 2024

yes!

@KnorpelSenf
Copy link
Contributor Author

Awesome, will do!

@KnorpelSenf
Copy link
Contributor Author

@lovasoa I've noticed that russcip is able to ship a precompiled binary. Wouldn't it be much simpler if good_lp allowed developers to leverage this so that they don't have to install scip themselves? If yes, this would also drastically simplify the good_lp setup as we wouldn't have to install conda first.

@lovasoa
Copy link
Collaborator

lovasoa commented Dec 25, 2024

@mmghannam what do you think ? What's the best way to depend on russcip from good-lp ?

@mmghannam
Copy link
Contributor

Yes, I agree with @KnorpelSenf that it would simplify the developer experience greatly if another feature flag in good_lp would enable the bundled feature in russcip. I still wouldn't remove the current feature flag as that enables linking to a custom SCIP installation (or linking through conda).

@lovasoa
Copy link
Collaborator

lovasoa commented Dec 25, 2024

Godd, let's do it that way: add a new flag to goodl-lp and use it on ci

@KnorpelSenf
Copy link
Contributor Author

Nice! Please check out #72

@KnorpelSenf
Copy link
Contributor Author

@lovasoa this is ready, please TAL

@lovasoa
Copy link
Collaborator

lovasoa commented Dec 26, 2024

Do we really have to add #[cfg(any(feature = "scip", feature = "scip_bundled"))] everywhere we depend on scip? Can we have a base feature we test against?

@KnorpelSenf
Copy link
Contributor Author

How about this?

Cargo.toml Outdated Show resolved Hide resolved
@KnorpelSenf
Copy link
Contributor Author

Also, something is still wrong with the container setup but I don't see it.

If you open the container and run cargo build, it fails with

$ cargo build
    Updating crates.io index
  Downloaded fnv v1.0.7
error: failed to open `/usr/local/cargo/registry/cache/index.crates.io-6f17d22bba15001f/fnv-1.0.7.crate`

Caused by:
  Permission denied (os error 13)

@KnorpelSenf
Copy link
Contributor Author

Seems to be related to the cargo install command in the setup script, let me see how this can be fixed

@KnorpelSenf
Copy link
Contributor Author

Done.

@lovasoa lovasoa merged commit 5bd41e9 into rust-or:main Dec 26, 2024
1 check passed
@lovasoa
Copy link
Collaborator

lovasoa commented Dec 26, 2024

Thank you!

@KnorpelSenf KnorpelSenf deleted the devcontainers branch December 26, 2024 21:45
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.

3 participants