NASA-SW-VnV / homebrew-core

Homebrew formulae from NASA - Software Verification and Validation
5 stars 3 forks source link

Homebrew formula improvements #9

Closed arthaud closed 11 months ago

arthaud commented 11 months ago

I looked into the new homebrew formula: https://github.com/ivanperez-keera/homebrew-core/blob/master/Formula/ikos.rb There are a few things I would like to address:

This is what I think we should do:

I don't have time to try today but I could try later this week.

ivanperez-keera commented 11 months ago

I haven't had the chance to read https://docs.brew.sh/Python-for-Formula-Authors just yet but I have made the changes you suggested in the formula.

I tried it in a docker image and it works. ikos seems to work well inside (a basic example runs and reports errors in the C code as expected).

ivanperez-keera commented 11 months ago

Btw, I'm holding off on sending a PR or merging the changes into this repo until the release is final.

ivanperez-keera commented 11 months ago

We should also require "python@3.x" here:

https://github.com/NASA-SW-VnV/homebrew-core/blob/cb8061cce95e6c76ca408d66da26295f96178536/Formula/ikos.rb#L18

Why do we put things in libexec/vendor? The only mention of vendor is in:

Dependencies for libraries

Library dependencies must be installed so that they are importable. To minimise the potential for linking conflicts, dependencies should be installed to libexec/ and added to sys.path by writing a second .pth file (named like “homebrew-foo-dependencies.pth”) to the prefix site-packages.

Note that, in the guide, <vendor> has angle brackets, indicating that it's not the literal string vendor. But I admit that it's a bit strange to customize <vendor> for each dependency or something like that.

ivanperez-keera commented 11 months ago

Also, while we are at it, should the license be NOSA?

https://github.com/NASA-SW-VnV/homebrew-core/blob/24cbcc2dfc254f27e2ffcebd56f6f670c709b05b/Formula/ikos.rb#L7

ivanperez-keera commented 11 months ago

We may not want to fight all of these "battles" for this release. Some of these concerns, even if appropriate, were there before 3.1 and do not prevent us from releasing. IKOS 3.3 will be out 3 months from now, so there's no need to squeeze as many fixes/features as possible in this release.

ivanperez-keera commented 11 months ago

I went through the guide and I could not find anything in the formula that contradicts the guide, apart from the small comment regarding <vendor>.

ivanperez-keera commented 11 months ago

Actually, forget about the vendor thing. First, that was added to the guide as part of a formatting commit (here), but I see nothing in the documentation that reflects that it is meant to mean "us". The parent commit uses libexec/"vendor", and the change is not clearly justified, so I would not be surprised if that was a mistake.

Second, looking through github shows that it's common enough for people to use libexec/"vendor". Since it works and I have no better proposal, let's ignore it altogether.

arthaud commented 11 months ago

Right, it should be NOSA instead of MIT.

Regarding the "vendor" thing, yeah I think this can go away. I see other formulas just do virtualenv_create(libexec, python3)

ivanperez-keera commented 11 months ago

Ok, I'll change all three things: python3, NOSA, and the virtualenv_create.

ivanperez-keera commented 11 months ago

I've made these changes, @arthaud . If nothing else is needed, I'll merge these into master.

If you have a chance to try it, please let me know how it goes.

ivanperez-keera commented 11 months ago

I'll update this with a new version number, and merge tomorrow Thursday if all goes according to plan. If you need to do any last-minute checks, @arthaud , please go ahead.