search

TaKO8Ki / frum

A little bit fast and modern Ruby version manager written in Rust
MIT License
629 stars 15 forks source link

Add `configure_opts` #56

Closed citizen428 closed 3 years ago

citizen428 commented 3 years ago

Closes #50

This PR adds a configure_opts arg that gets passed through to the ./configure call unmodified.

This uses a TrailingVarArg to collect options in a multiple arg. It supports the following two forms:

frum install 3.0.1 --with-jemalloc # only when no flags are specified
frum install 3.0.1 -- --with-jemalloc # always

I'm opening this as a draft PR for now to get feedback on two questions:

  1. Are you ok with this approach @TaKO8Ki? I originally just added a with-jemalloc arg but this has the potential for becoming unwieldy so It may make more sense to allow for arbitrary arguments to be passed through.
  2. Should we remove the dedicated with-openssl-dir option and handle it through the same mechanism?

Once we decided on this I'll finish off this PR and fix the tests (they will fail for the completions alone).

changelog: Add configure_opts to pass through to ./configure.

TaKO8Ki commented 3 years ago

Thank you for your contribution! I'm going to review this PR within today or tomorrow.

citizen428 commented 3 years ago

No rush @TaKO8Ki! It's not done yet anyway, as soon as you have time to answer the two questions I posted above I will fix the specs and do any other outstanding work.

TaKO8Ki commented 3 years ago

@citizen428 Ok. I'm going to answer the two questions within today or tomorrow!

TaKO8Ki commented 3 years ago

@citizen428

Sorry for the late reply.

Are you ok with this approach @TaKO8Ki? I originally just added a with-jemalloc arg but this has the potential for becoming unwieldy so It may make more sense to allow for arbitrary arguments to be passed through.

Yes. It would be ideal to implement all configure options and completions for them, but it seems difficult because configure has many options and different versions probably have different options. So, at this stage, I think your approach is the best!

Should we remove the dedicated with-openssl-dir option and handle it through the same mechanism?

Yes. Please do so.

citizen428 commented 3 years ago

@TaKO8Ki This should be ready for review now.

Edit: seems like there are some failing tests, I probably won't have time to look into that until tomorrow.

citizen428 commented 3 years ago

@TaKO8Ki I added the default value for the OpenSSL directory back in.

TaKO8Ki commented 3 years ago

@citizen428 Thank you for working on this. I left some suggestions. Then could you add e2e tests for configure_opts to https://github.com/TaKO8Ki/frum/blob/main/tests/e2e/mod.rs? I'm sure you can guess how to add e2e tests from other tests, but if you have any questions, please ask me.

citizen428 commented 3 years ago

Thanks for the feedback, this is all quite helpful as I'm relatively new to Rust.

citizen428 commented 3 years ago

Right, I seem to have some issues with the e2e test, if you could take a look that would be helpful 😒

TaKO8Ki commented 3 years ago

@citizen428 I'm sorry for the merge conflict.

citizen428 commented 3 years ago

@TaKO8Ki No problem at all, it's sort of my own fault. I wanted to finish this up yesterday, than had a busy day and forgot.

citizen428 commented 3 years ago

@TaKO8Ki Finally ready, sorry this took so long.

TaKO8Ki commented 3 years ago

@citizen428

Finally ready, sorry this took so long.

Don't worry about it :)

citizen428 commented 3 years ago

Given this branch was quite messy (sorry for that, I had resolved the merge conflicts in the browser when I wasn't on my main computer, clearly that went wrong) it's probably best to squash this one on merge.