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

oil 0.7.0 (new formula) #51962

Closed
wants to merge 1 commit into from
Closed

Conversation

waldyrious
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@waldyrious
Copy link
Contributor Author

waldyrious commented Mar 20, 2020

@andychu, for the formula's test block I used the "smoke test" section of https://www.oilshell.org/release/latest/doc/INSTALL.html for inspiration, but at some point during local testing, the produced AST for the demo script I was using changed as follows:

-       spids: [5 18]
+       spids: [4 17]

Any insights as to what could explain this change? I don't want to introduce a test that isn't deterministic.

@andychu
Copy link

andychu commented Mar 21, 2020

-n is more of an informal demo for users -- I wouldn't use that as the test. That format is subject to change at any time (and maybe I should make that more clear somehow)

Also despite the versioning, the later releases are almost certainly strictly better than earlier ones, so

https://www.oilshell.org/release/0.8.pre2/

is worth packaging.

Thanks for packaging Oil! Please update the wiki when it's merged and let me know if you have more questions.

https://github.com/oilshell/oil/wiki/Oil-Deployments

@andychu
Copy link

andychu commented Mar 21, 2020

Oh also eventually Oil will be distributed with tests you can run. That's this issue:

oils-for-unix/oils#167

If you want to help let me know :) There's been some big progress recently at http://travis-ci.oilshell.org/jobs/ (details on Zulip)

@chenrui333
Copy link
Member

@andychu just curious why this oilshell depends on Python 2, not Python 3. Also, depends on python 2.7.13 (not 2.7.16)

Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bring this shell into my attention, good to know there is always another one exist. :)

@andychu
Copy link

andychu commented Mar 21, 2020

The goal is to remove the CPython dependency altogether. Some details:

http://www.oilshell.org/blog/2020/01/parser-benchmarks.html

http://www.oilshell.org/blog/2019/06/17.html#why-is-it-written-in-python

@SMillerDev
Copy link
Member

If it depends on Python it should define uses_from_macos "python@2"

@andychu
Copy link

andychu commented Mar 21, 2020

It doesn't require Python to be installed: From the packager's perspective it looks like a C program.

@SMillerDev
Copy link
Member

Does it require python to run?

@waldyrious
Copy link
Contributor Author

-n is more of an informal demo for users -- I wouldn't use that as the test.

What would you suggest instead, then? Perhaps one of the spec tests?

despite the versioning, the later releases are almost certainly strictly better than earlier ones, so oilshell.org/release/0.8.pre2 is worth packaging.

Sure, I can do that. @SMillerDev, @chenrui333, is that OK from your side as well?

@andychu
Copy link

andychu commented Mar 21, 2020

I would just remove it -- -c "echo hi" is a good sanity check already. And as mentioned Oil issue 167 is for people who want more than that.

@SMillerDev It doesn't require Python to run

@waldyrious
Copy link
Contributor Author

Thanks for packaging Oil! Please update the wiki when it's merged

No problem, and will do 👍

@SMillerDev
Copy link
Member

SMillerDev commented Mar 21, 2020

is that OK from your side as well?

Nope, only stable releases are allowed.

@SMillerDev It doesn't require Python to run

Okay.

@waldyrious
Copy link
Contributor Author

Another question: is there any way to link to the source code repository of the formula when it has a dedicated website? I know I could use head, but IIUC that implies that the formula should support building from the master branch, which AFAIK is not the case — according to the README:

Developer builds don't work on OS X, so use the release tarballs on OS X.

@SMillerDev
Copy link
Member

You can use head for that. While it allows people to try and build the software, that configuration means that you're on your own if it breaks.

@waldyrious
Copy link
Contributor Author

I've added the head block and simplified the test. I also made a minor adjustment to the head block of another formula, but I can submit that as a separate PR if it's preferable.

Formula/gobby.rb Outdated
@@ -4,7 +4,7 @@ class Gobby < Formula
url "http://releases.0x539.de/gobby/gobby-0.5.0.tar.gz"
sha256 "8ceb3598d27cfccdf9c9889b781c4c5c8e1731ca6beb183f5d4555644c06bd98"
revision 7
head "https://github.com/gobby/gobby"
head "https://github.com/gobby/gobby.git"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SMillerDev I suspect I'll need to also fix this if I am to submit that change, but I'm not sure exactly how to do that.

Can you clarify what adjustment I would need to do to gobby/0.5.0.patch, and whether that would need to be done before or after the PR in this repo?

@waldyrious
Copy link
Contributor Author

Fixed the audit error and removed the change to an unrelated formula.

@SMillerDev
Copy link
Member

If you can just squash the commits into one <name> <version> (new formula) now?

@SMillerDev SMillerDev added the almost there PR is nearly ready to merge label Mar 21, 2020
@waldyrious
Copy link
Contributor Author

Sure, I wasn't sure if force-pushing was OK. Will squash now.

@chenrui333 chenrui333 added ready to merge PR can be merged once CI is green and removed almost there PR is nearly ready to merge labels Mar 24, 2020
@chenrui333
Copy link
Member

@waldyrious Thanks for your contribution to Homebrew! 🎉 🥇

Without awesome contributors like you, it would be impossible to maintain Homebrew to the high level of quality users have come to expect. Thank you!!!!

@lock lock bot added the outdated PR was locked due to age label Apr 25, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants