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

sile 0.10.3 #49744

Closed
wants to merge 1 commit into from
Closed

sile 0.10.3 #49744

wants to merge 1 commit into from

Conversation

alerque
Copy link
Contributor

@alerque alerque commented Feb 3, 2020

  • 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>)?

@alerque
Copy link
Contributor Author

alerque commented Feb 3, 2020

@alerque alerque changed the title Sile v0.10.2 Sile v0.10.3 Feb 4, 2020
@alerque
Copy link
Contributor Author

alerque commented Feb 5, 2020

@simoncozens I attempted a fix here for the Lua path issue. I think the wrapper script should now just prepend its own paths instead of completely clobbering what Lua would otherwise have had. I am completely unable to test this though so I'm leaving the PR in draft mode for now until there is some confirmation that it works.

@alerque
Copy link
Contributor Author

alerque commented Feb 5, 2020

If this works, another thing to try and simplify (and avoid the Lua invocations) is this:

diff --git a/Formula/sile.rb b/Formula/sile.rb
index 3909af5e6b..90501c13ce 100644
--- a/Formula/sile.rb
+++ b/Formula/sile.rb
@@ -139,8 +139,8 @@ class Sile < Formula
     (libexec/"bin").install bin/"sile"
     (bin/"sile").write <<~EOS
       #!/bin/bash
-      export LUA_PATH="#{ENV["LUA_PATH"]};$(lua -e 'print(package.path)')"
-      export LUA_CPATH="#{ENV["LUA_CPATH"]};$(lua -e 'print(package.cpath)')"
+      export LUA_PATH="#{ENV["LUA_PATH"]};;"
+      export LUA_CPATH="#{ENV["LUA_CPATH"]};;"
       "#{libexec}/bin/sile" "$@"
     EOS
   end

The double ;; should be a shortcut to tell Lua to add it's default paths, but I was unsure if that worked from the environment variable or only internally after it was already running.

@simoncozens
Copy link
Contributor

PR as written works fine. Checking the suggested patch now.

@simoncozens
Copy link
Contributor

Double-semicolon solution also seems to work.

@alerque
Copy link
Contributor Author

alerque commented Feb 5, 2020

@simoncozens Thanks. The double-semicolon trick is less process overhead so we'll go with that.

@alerque alerque marked this pull request as ready for review February 5, 2020 14:06
@SMillerDev
Copy link
Member

Looks good, can you squash your commits?

@alerque
Copy link
Contributor Author

alerque commented Feb 6, 2020

@SMillerDev Done.

@alerque alerque changed the title Sile v0.10.3 sile 0.10.3 Feb 6, 2020
@alerque
Copy link
Contributor Author

alerque commented Feb 6, 2020

It seems like CI got stuck, it has been "pending" for a day. I'm rebasing on master just to poke it (since I don't have any other way of triggering it to retry).

* Bump version to latest upstream release
* Add missing Luarock dependency
* Require git history for HEAD builds
* Prefix rather than occlude Lua paths in wrapper

Co-authored-by: Simon Cozens <simon@simon-cozens.org>
@alerque
Copy link
Contributor Author

alerque commented Feb 6, 2020

Hey looks like that worked. If there is a different way to un-jam CI checks that never return I'd be happy to learn.

@SMillerDev
Copy link
Member

You can ask brewtestbot to test this please

@alerque alerque requested a review from chenrui333 February 8, 2020 07:26
@chenrui333
Copy link
Member

Thanks @alerque!!

@chenrui333 chenrui333 added the ready to merge PR can be merged once CI is green label Feb 11, 2020
@bayandin bayandin closed this in c918036 Feb 11, 2020
@alerque alerque deleted the sile-v0.10.2 branch February 11, 2020 11:28
@lock lock bot added the outdated PR was locked due to age label Mar 12, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 12, 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