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

ltex-ls 15.2.0 (new formula) #95817

Closed
wants to merge 4 commits into from

Conversation

gzagatti
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • 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 --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@BrewTestBot BrewTestBot added java Java use is a significant feature of the PR or issue new formula PR adds a new formula to Homebrew/homebrew-core labels Feb 25, 2022
@gzagatti
Copy link
Contributor Author

CI raises the following error:

  Error:  Failed to execute goal org.apache.maven.plugins:maven-antrun-plugin:3.0.0:run (add-ltex-cli) on project ltexls: An Ant BuildException has occured: Execute failed: java.io.IOException: Cannot run program "python" (in directory "/tmp/ltex-ls-20220225-20481-8duwhb/ltex-ls-15.2.0"): error=2, No such file or directory
  Error:  around Ant part ...<exec executable="python">... @ 4:31 in /tmp/ltex-ls-20220225-20481-8duwhb/ltex-ls-15.2.0/target/antrun/build-main.xml
  Error:  -> [Help 1]

However, I'm not able to reproduce it in my local machine as the formula builds with no problem.

depends_on "openjdk@11"

def install
ENV["JAVA_HOME"] = Formula["openjdk@11"].opt_prefix
Copy link
Member

Choose a reason for hiding this comment

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

it seems this relies on a python executable being available: https://github.com/valentjn/ltex-ls/blob/4954220ebd34df6c032ca646fd9bab5bb4d8231e/pom.xml#L376

we can add one to PATH:

Suggested change
ENV["JAVA_HOME"] = Formula["openjdk@11"].opt_prefix
ENV.prepend_path "PATH", Formula["python@3.9"].opt_libexec/"bin"
ENV["JAVA_HOME"] = Formula["openjdk@11"].opt_prefix

head "https://github.com/valentjn/ltex-ls.git", branch: "develop"

depends_on "maven" => :build
depends_on "python@3.9" => :build
Copy link
Member

Choose a reason for hiding this comment

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

can this use python@3.10?

Suggested change
depends_on "python@3.9" => :build
depends_on "python@3.10" => :build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion, they were very illuminating. I've implemented them. Let's see whether CI passes now.

@BrewTestBot BrewTestBot removed the new formula PR adds a new formula to Homebrew/homebrew-core label Feb 26, 2022
@BrewTestBot BrewTestBot added the new formula PR adds a new formula to Homebrew/homebrew-core label Feb 26, 2022
@gzagatti
Copy link
Contributor Author

The problem with the new build is that the command

system Formula["python@3.10"].bin/"python3", "-u", "tools/createCompletionLists.py"

Attempt to write completion lists in a temporary folder which is then unwrittable in CI. The source code is here

Invalid argument: Path is not writable: /var/folders/07/mmpkd6_x7fvbpl_msv10c7fh0000gp/T

Since the completion lists fail to write, the maven unit test that requires it then fails. I'm not sure if the completion lists are required for the application to work or it's only temporarily created for the unit tests.

In any case, I am pushing a build that does not execute the tests. Let's see how it does.

@gzagatti
Copy link
Contributor Author

It turns out the completion list is necessary, but I figure out that by changing the environment variable TMPDIR in the build, I can get the completion list script to produce temporary directories in the build directory.

@gzagatti
Copy link
Contributor Author

The problem in CI seems to be unrelated with the build process:

  Error: ltex-ls: no bottle available!
  You can try to install from source with:
    brew install --build-from-source ltex-ls
  Please note building from source is unsupported. You will encounter build
  failures with some formulae. If you experience any issues please create pull
  requests instead of asking for help on Homebrew's GitHub, Twitter or any other
  official channels.

I'm not sure how to overcome this. Any help would be appreciated.

@cho-m cho-m added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Mar 2, 2022
@gzagatti
Copy link
Contributor Author

gzagatti commented Mar 7, 2022

The two CI processes fail because there is no bottle available. How can I create a bottle @branchvincent?

@branchvincent
Copy link
Member

there is no bottle available

hmm, this is a confusing error but the reason is that openjdk@11 isn't bottled on Monterey. If it's possible, we can try building with openjdk

@gzagatti
Copy link
Contributor Author

gzagatti commented Mar 9, 2022

@branchvincent thanks for your comment. It was super helpful. I've made the modifications and the formula can build with openjdk without a problem. I hope that it now passes all CI test.

@SMillerDev
Copy link
Member

  Invalid argument: Path is not writable: /var/folders/sd/fmj8vmcx7f32h_wz5tn7dy4c0000gp/T

It's trying to write outside of the install prefix

@gzagatti
Copy link
Contributor Author

gzagatti commented Mar 9, 2022

I forgot to fix JAVA_HOME when I updated openjdk. I’m glad that fixing it solved all the issues.

@gzagatti
Copy link
Contributor Author

I'm just wondering if there is any expectation for review of this PR?

Formula/ltex-ls.rb Outdated Show resolved Hide resolved
Formula/ltex-ls.rb Outdated Show resolved Hide resolved
Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>
@gzagatti gzagatti closed this Mar 18, 2022
@gzagatti gzagatti reopened this Mar 18, 2022
Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>
@gzagatti
Copy link
Contributor Author

Thanks for revision! I closed the issue by mistake (the page refreshed with the mouse right on top of the close button :( ) and then re-opened.

Formula/ltex-ls.rb Outdated Show resolved Hide resolved
Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

Thanks @gzagatti ! Without contributions like yours it'd be impossible to keep homebrew going with the high standards that users have come to expect from the project. You can feel good knowing that you've made the world a tiny bit better for homebrew users around the world! 👍 🎉

@gzagatti
Copy link
Contributor Author

That's great news! Thanks for taking the time of reviewing the new formula and for maintaining Homebrew for the benefit of all. This is one piece of software that has followed me through my whole computer journey.

@BrewTestBot
Copy link
Member

:shipit: @SMillerDev has triggered a merge.

@gzagatti gzagatti deleted the new-formula-ltex-ls branch April 29, 2022 09:58
@github-actions github-actions bot added the outdated PR was locked due to age label May 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. java Java use is a significant feature of the PR or issue new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants