homebrew-core icon indicating copy to clipboard operation
homebrew-core copied to clipboard

xdk 0.4.2 (new formula)

Open cpurdy opened this issue 2 years ago • 18 comments

Note: was previously Homebrew#104232

  • [x] Have you followed the guidelines for contributing?
  • [x] Have you ensured that your commits follow the commit style guide?
  • [x] Have you checked that there aren't other open pull requests for the same formula update/change?
  • [x] Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • [x] Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • [x] 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>?

I have not been able to get this to pass "brew test" for environment reasons mentioned on a previous thread. Everything else seems to be working well, so my hope is that the environment issue is specific to my workstation.

cpurdy avatar Jun 25 '22 02:06 cpurdy

Goodness gracious:

Error: Final newline missing.

The first time I submitted a formula, I got this error. So I added a final newline and re-submitted the formula, and I got a different error stating that the newline should not be there. I can't tell if this is a joke or something; it should either be an error to have the final newline, or it should be an error to not have the final newline, but both of those should not be an error.

cpurdy avatar Jun 25 '22 03:06 cpurdy

Goodness gracious:

Error: Final newline missing.

The first time I submitted a formula, I got this error. So I added a final newline and re-submitted the formula, and I got a different error stating that the newline should not be there. I can't tell if this is a joke or something; it should either be an error to have the final newline, or it should be an error to not have the final newline, but both of those should not be an error.

it is not a joke, it is a code style alert. (adding a newline would be a simple fix)

chenrui333 avatar Jun 25 '22 04:06 chenrui333

also this formula does not build on m1 machines.

chenrui333 avatar Jun 25 '22 04:06 chenrui333

Hi Rui, I am running on an M1 (2021 MBP M1 Max). What error are you seeing on an M1? Or is this something you saw on the testing infrastructure? Sorry if these questions seem silly; I am still learning this process.

cpurdy avatar Jun 25 '22 14:06 cpurdy

This is the same error that I got locally:

/opt/homebrew/Cellar/xdk/0.4.2/bin/xec: line 2: /bin/java: No such file or directory

Could someone help me to figure out how ${JAVA_HOME} is blank? It only happens in brew test mode, and it happens in the shell script created by Homebrew that explicitly avoids the possibility of ${JAVA_HOME} being blank:

#!/bin/bash
JAVA_HOME="${JAVA_HOME:-/opt/homebrew/opt/openjdk/libexec/openjdk.jdk/Contents/Home}" exec "${JAVA_HOME}/bin/java" ...

The script is created with this line of Ruby code in the Formula:

java = "${JAVA_HOME}/bin/java"
...
(bin/cmd).write_env_script java, args, Language::Java.overridable_java_home_env

Since I don't full understand what brew test is doing under the covers, is it possible that it's creating a different shell script somewhere in a temporary test location that does NOT match what I see in /opt/homebrew? And that when it does so, it somehow loses the ${JAVA_HOME} and creates a bogus script? Is there some other way that the script should be invoking Java, instead of ${JAVA_HOME}/bin/java?

--

Regarding the other issue flagged: Why would shipping universal binaries be a problem?

Unexpected universal binaries were found.

Some time back, we switched to universal binaries for macOS to try to make people's lives easier. Why would we want to switch back to the 1990s model? We have one distribution image, and that has to cover Windows, macOS, and Linux.

cpurdy avatar Jun 25 '22 15:06 cpurdy

Could someone help me to figure out how ${JAVA_HOME} is blank?

Because it's never set by CI.

Regarding the other issue flagged: Why would shipping universal binaries be a problem?

Because it's not needed. Homebrew knows how to ship the correct binaries for the correct architecture. Why work around that optimization?

SMillerDev avatar Jun 25 '22 16:06 SMillerDev

Because it's never set by CI.

But that is simply not true. (Hence the large amount of time that I have spent struggling with this.)

On my local macOS machine (both M1 and x86) and Linux machines (both M1 and x86), $JAVA_HOME has a value.

Furthermore, the script produced by Homebrew using write_env_script explicitly encodes a literal value for $JAVA_HOME, just in case it is blank. It uses the ":-" shell feature. That seems like it is supposed to be a fail-safe!

Yet, on my machine, where I can see both $JAVA_HOME (with a value) and the text of the script produced by Homebrew with a default literal value for $JAVA_HOME, the brew test has no $JAVA_HOME.

The script that Homebrew emits has this line:

 JAVA_HOME="${JAVA_HOME:-/opt/homebrew/opt/openjdk/libexec/openjdk.jdk/Contents/Home}" exec "${JAVA_HOME}/bin/java" ...

If I change it manually to:

JAVA_HOME="${JAVA_HOME:-/opt/homebrew/opt/openjdk/libexec/openjdk.jdk/Contents/Home}" exec "${JAVA_HOME:-/opt/homebrew/opt/openjdk/libexec/openjdk.jdk/Contents/Home}/bin/java" ...

Then it works. But that seems super wrong (and non-obvious how) to do that from my Formula, though. So I did that from vi after the brew test failed. I am just trying to be super clear here: The syntax used by Homebrew in the Homebrew-generated portion of script emitted by write_env_script does not work. It does not work on my machine. It does not work on the CI machine. That seems like an obvious bug.

After my fifth double espresso so far today, this is the solution that I am planning to push:

JAVA_HOME="${JAVA_HOME:-/opt/homebrew/opt/openjdk/libexec/openjdk.jdk/Contents/Home}" exec "${JAVA_HOME:+$JAVA_HOME/bin/}java" ...

Just to be clear: I have changed the formula (in a completely non-obvious way) in order to compensate for the way that the brew test carefully erases the $JAVA_HOME variable, and the Homebrew-emitted script incorrectly fills in as "" -- even though the literal value is pasted by Homebrew into the script as a default, but due to the bug in the automatically emitted code, it does not "stick"!

I believe that this is the same problem that I saw a few years back when Bash changed how sourcing (and export) works. My hypothesis is that the setting of the JAVA_HOME variable (by the Homebrew emitted code) in the script is lost by the time that the script advances to the "exec" command! If I can find a general solution to this bug, I'll propose it.

cpurdy avatar Jun 25 '22 17:06 cpurdy

I have pushed up to my fork, but I do not know what I need to do to update the pull request. Is it automatic? Here is the latest push to my fork:

https://github.com/xtclang/homebrew-core/commit/0febb797c3f3474c70d322db3ccaffe67dbc5dcd

The brew test is now working locally.

--

homebrew$ brew test xdk
==> Testing xdk
==> /opt/homebrew/Cellar/xdk/0.4.2/bin/xec -help
homebrew$ brew audit --strict --online xdk
homebrew$ 

cpurdy avatar Jun 25 '22 18:06 cpurdy

homebrew PR flow does not allow merge from master, and you should use some branch other master for creating the PR. Can you rebase to the upstream master and squash the commits to do a forced update? Then I can pull the commit for my local testing.

chenrui333 avatar Jun 26 '22 02:06 chenrui333

I figured out that it had to be in the master branch of the fork, because apparently that is how I set up the original pull request. Glad to see that all the tests are passing now.

cpurdy avatar Jun 27 '22 01:06 cpurdy

@SMillerDev I have submitted some clarifying changes. Hopefully this clears things up, in terms of why I was unable to use the write_jar_script. My use case may be fairly uncommon, but if it is not, I could imagine an optional parameter being added to write_jar_script allowing program arguments to be appended after the jar name.

One other question, which I could not figure out despite reading through much code. Why did I have to do the chmod myself, when the various helper methods (like write_jar_script) do not? The shell scripts that I emit in the bin directory are not marked as executable, unless I explicitly chmod them.

cpurdy avatar Jun 27 '22 13:06 cpurdy

Hopefully this clears things up, in terms of why I was unable to use the write_jar_script.

The current version seems to do the same as my suggestion did, just with an explicit max memory size and much harder to maintain. What errors are you getting when you use my last suggestion?

Why did I have to do the chmod myself, when the various helper methods (like write_jar_script) do not?

Because they exist to help write scripts, it's already part of the code.

SMillerDev avatar Jun 27 '22 16:06 SMillerDev

The current version seems to do the same as my suggestion did, just with an explicit max memory size and much harder to maintain. What errors are you getting when you use my last suggestion?

I appreciate that you are giving of your time to help me, so I want to be very respectful of that. I definitely do not want you to be using your scarce time to test and/or debug my code, or to have to learn and understand each of the technical nuances of arcane b.s. that I must know to do my job. I am more than happy to answer your questions, but I do not want to waste your time.

Here are the errors from your script suggestion:

Error: xdk-test: /Users/cameron/Development/homebrew-xvm/xdk-test.rb:16: syntax error, unexpected tIDENTIFIER, expecting end
.../"bin".write_jar_script libexec/"javatools/javatools.jar", c...
...                        ^~~~~~~
/Users/cameron/Development/homebrew-xvm/xdk-test.rb:16: syntax error, unexpected ',', expecting end
...exec/"javatools/javatools.jar", cmd, "#{cmd} -L #{libexec}/l...
...                              ^
/Users/cameron/Development/homebrew-xvm/xdk-test.rb:16: syntax error, unexpected '\n', expecting '.' or &. or :: or '['
/Users/cameron/Development/homebrew-xvm/xdk-test.rb:37: syntax error, unexpected end, expecting end-of-input

But there was an easy fix:

(libexec/"bin").write_jar_script ...

And that produces the following exec line, if I cat out the shell script:

exec "${JAVA_HOME}/bin/java" xec -L /opt/homebrew/Cellar/xdk-test/0.4.2/libexec/lib -L /opt/homebrew/Cellar/xdk-test/0.4.2/libexec/javatools -jar "/opt/homebrew/Cellar/xdk-test/0.4.2/libexec/javatools/javatools.jar" "$@"

Then, when I try to run the xec command, for example, it produces:

zsh: permission denied: xec

OK, this is really weird: The shell script is not marked as executable. (I asked about this ... I couldn't figure out who was supposed to be marking the script as executable. See above.) Anyway, it was easy enough to see that problem and fix it from the shell:

homebrew$ ls -al /opt/homebrew/Cellar/xdk-test/0.4.2/libexec/bin/xec 
-rw-r--r--  1 cameron  admin  326 Jun 27 15:14 /opt/homebrew/Cellar/xdk-test/0.4.2/libexec/bin/xec
homebrew$ chmod +x /opt/homebrew/Cellar/xdk-test/0.4.2/libexec/bin/xec
homebrew$ ls -al /opt/homebrew/Cellar/xdk-test/0.4.2/libexec/bin/xec
-rwxr-xr-x  1 cameron  admin  326 Jun 27 15:14 /opt/homebrew/Cellar/xdk-test/0.4.2/libexec/bin/xec
homebrew$ 

And now we can run the commands (e.g. "xec") and see that they are completely broken:

homebrew$ xec
Error: Could not find or load main class xec
Caused by: java.lang.ClassNotFoundException: xec

With the java command, and specifically when using the -jar option, the order that the various things appear on the command line is critically important; you can see this when you type java --help:

java [options] -jar <jarfile> [args...]

Any bits to the left of the -jar option get gobbled by the JVM as JVM options, while everything to the right of the -jar option is passed to the public static void main(String[]) method as arguments. I very carefully broke these out in my latest formula so you would see these details related to the order:

exec "#{java_bin}" #{java_opts} -jar #{java_jar} #{cmd} -L #{libexec}/lib -L #{libexec}/javatools "$@"
                                ^^^^

Specifically, the portion #{cmd} -L #{libexec}/lib -L #{libexec}/javatools "$@" needs to go to the main method as arguments.

Please trust me that I do not want to place my own cut&paste garbage into my formula; I would much rather use helper functions provided by the Homebrew project if they meet the requirements for what my formula must do. I have been reading and learning many thousands of lines of Homebrew code just to understand what is going on behind the scenes, when things do not work the way that I need them to. (And I am contributing back documentation edits to try to help others who may fall into the same holes that I did, because having to read all of the code to understand what is going on is not efficient.)

Let me know if you have more questions or suggestions.

cpurdy avatar Jun 27 '22 19:06 cpurdy

Is there some "next step" for getting this pull request approved? This is my first time doing this, so I am not sure what to expect w.r.t. to the timeline, etc.

cpurdy avatar Jun 29 '22 13:06 cpurdy

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Jul 21 '22 00:07 github-actions[bot]

I'm still curious if there is something that I'm supposed to do to get this in.

cpurdy avatar Jul 21 '22 12:07 cpurdy

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Aug 11 '22 13:08 github-actions[bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Sep 13 '22 18:09 github-actions[bot]

I just realized that even though this was "approved" on this thread, that it never got merged. Was there something that I had to do to make that happen?

cpurdy avatar Sep 21 '22 02:09 cpurdy

I just realized that even though this was "approved" on this thread, that it never got merged. Was there something that I had to do to make that happen?

mostly there are review comments need to be addressed.

@cpurdy do you mind creating a new one?

chenrui333 avatar Dec 30 '22 19:12 chenrui333

Yes, I can do that. I will try to "update" a few things accordingly this time, though, to avoid some of the issues we had last time around. We're in the middle of a few big time-suck projects ATM, so this will probably take a few days/weeks to get rolling.

cpurdy avatar Jan 02 '23 18:01 cpurdy