-
-
Notifications
You must be signed in to change notification settings - Fork 382
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
Add scalalib.ApeModule to produce APE (actually portable executable) assembly jar #4503
base: main
Are you sure you want to change the base?
Conversation
e8dc45c
to
df9f091
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't tell what's the best name, but Ape
feels a bit cryptic. It's IMHO not a well established term. Since we seem to use just one tool/technology jere, we might as well name the module after it. This is what we do in many other modules and it also leaves room for alternative implementations for APE. CosmoModule
might be the better choice.
PathRef(Task.dest / "cosmocc" / "bin" / "cosmocc") | ||
} | ||
|
||
def launcherScript: T[Option[String]] = Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a prefix. Either ape
or cosmo
.
} | ||
} | ||
|
||
def assemblyApe: T[PathRef] = Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cosmoAssembly
or apeAssembly
. Depends on which name we choose above. Should be consistent though.
destJar: os.Path, | ||
inputPaths: Agg[os.Path], | ||
manifest: mill.api.JarManifest = mill.api.JarManifest.MillDefault, | ||
prependShellScript: Option[String] = None, | ||
prepend: Option[os.Source] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you need a byte array instead of a string. Woud be a Option[String|Array[Byte]]
sufficient? Or do you expect large data here? A os.Source
feels a bit too generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option[String|Array[Byte]]
would work as well. The data is around 300K in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lihaoyi What do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.Source
does feel a bit unusual, I think a 300kb byte array is probably fine for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basic question: What is the simplest way to pass a x: String | Array[Byte]
to os.write
? Looks like there is no implicit conversion for unions. os.write(..., x)
does not work. x: os.Source
, x.asInstanceOf[os.Source]
doesn't work either. Is there any other way that's less clunky than x match { case x: String => os.write(..., x); case x: Array[Byte] => os.write(..., x) }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the answer. Could be, this isn't fully implemented in upickle. But we could also either fallback to always use a byte array, which sounds a bit inconvenient for the other cases, or go with a Option[Either[Array[Byte],String]]
for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just using byte array is fine. Strings can be converted with just .getBytes
, so it's no big hardship. Definitely easier than wrapping in Either
s and Left
s and Right
s
@lefou Thanks for the review. Admittedly this is just a POC PR I quickly drafted up to demonstrate the idea. I haven't thought too much about organizing things yet, which obviously needs fleshing out if this is to be developed further. Just want to mention a few drawbacks to this approach
|
In terms of naming, I think 300kb is fine; any Scala program gets 4-5mb from scala-library anyway, and most Java programs will have similar dependency trees. Mill's own executable assemblies bundle Coursier with Graal for a >60mb jar prepend and it works great We need to see what's going on with Powershell to see if it can be resolved. Currently Powershell is the default shell for Windows Terminal, and the whole point of APEs is to work seamlessly on Windows environments where previously you would need to rename One end-to-end test that should be done is to build the Mill |
@lihaoyi I figured out the problem with PowerShell. It looks like PowerShell strictly follows |
This section on
and I checked cmd /c ftype jarfile
jarfile="C:\Program Files\Java\jdk-17\bin\javaw.exe" -jar "%1" %* This means that if the user installs java using the Windows installer, which registers jarfile with ftype, and we could change the assembly zip to be able to run |
I don't like that or a shorter version |
@kiendang if the extension matters, what if we name the output file |
This is the default for PATHEXT: |
@lihaoyi Yup that would work. @sake92 I actually think @ all, I have a bit of problem doing this
One way of enabling
package build.dist
object `package` extends RootModule with InstallModule {
...
object ape
extends RootModule
with InstallModule
with mill.scalalib.ApeModule {
export `package`.{executableRaw as _, *}
override def executableRaw = ...
}
} but looks like mill doesn't support export clauses? If I do
also inside
Not sure if I miss something. |
@kiendang Let's go with @lefou's suggestion of I'm not sure about However, for now you can't use stuff defined in your PR in Mill's own |
Added the tests to I moved Right now where should I cache the Cosmo toolchain? I see #3930 but looks like nothing is standardized yet? |
Yes there's no standard global cache yet, so caching i the task.dest folder is fine for now |
scalalib/src/mill/scalalib/cosmopolitan/CosmopolitanModule.scala
Outdated
Show resolved
Hide resolved
scalalib/src/mill/scalalib/cosmopolitan/CosmopolitanModule.scala
Outdated
Show resolved
Hide resolved
requests.get.stream(s"https://cosmo.zip/pub/cosmocc/${versionedCosmocc}.zip") | ||
) | ||
os.remove.all(Task.dest / versionedCosmocc) | ||
os.call(("unzip", zip, "-d", Task.dest / versionedCosmocc)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use os.unzip
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did try os.unzip
but it doesn't preserve permissions. Would have to have an extra step to give cosmocc
executable permission. cosmoc.zip
also contains other files. I'm not sure if not having the original permissions changes anything (though unlikely).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with os.zip
then and os.perms.set
to fix anything necessary, and open an issue on OS-Lib about os.unzip
not preserving permissions so we can fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this doesn't look like a simple fix. With os.unzip
after giving executable perm to files inside cosmocc/bin
compilation still fails with
x86_64-linux-cosmo-gcc: fatal error: cannot execute 'cc1': posix_spawnp: No such file or directory
compilation terminated.
Looks like os.unzip
lose stuff beyond permissions, e.g. if I'm not wrong symlinks are not preserved but replaced with copied files, and I'm not sure what exactly fails the compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, using a subprocess is probably fine for now, but can you open an issie upstream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name you settled with is fine. But I'd like to have some more consistent naming. We now have the Cosmopolitan
, cosmo
and ape
as prefix. Would be nice to get rid of ape
in favor to cosmo
.
One more thing. The current shell script approach uses the The cleanest approach is to use java argument files, i.e. write the values of Another approach is to append There is a very small edge case with appending to In summary there are 3 approaches
|
Maybe it's possible after all: #include <stdio.h>
#include <stdlib.h>
#include <wordexp.h>
int main() {
char *env_var = getenv("JAVA_OPTS");
if (!env_var) {
printf("JAVA_OPTS is not set.\n");
return 1;
}
wordexp_t p;
if (wordexp(env_var, &p, 0) != 0) {
perror("wordexp failed");
return 1;
}
// Print parsed options
printf("Parsed JAVA_OPTS:\n");
for (size_t i = 0; i < p.we_wordc; i++) {
printf(" Arg[%zu]: %s\n", i, p.we_wordv[i]);
}
wordfree(&p); // Free allocated memory
return 0;
} While https://www.gnu.org/software/libc/manual/html_node/Wordexp-Example.html |
ef7c23e
to
b37159b
Compare
Looks good to me overall. Just some nits and docs and I think we can merge it |
@@ -0,0 +1,12 @@ | |||
package build | |||
|
|||
import mill._, javalib._, scalalib._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put an alias javalib.cosmo
so people don't need to import it from scalalib._
in Java projects
b985591
to
f71da73
Compare
Add an
ApeModule
to support producing actually portable executable assembly jars.out.jar
would run cross-platform (tested with arm Mac, x86 Linux and Windows (on cmd) with this example hello world project).