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

Update ammonite.json to point at new Main #125

Merged
merged 2 commits into from
Feb 1, 2022
Merged

Update ammonite.json to point at new Main #125

merged 2 commits into from
Feb 1, 2022

Conversation

blandflakes
Copy link
Contributor

@blandflakes blandflakes commented Dec 1, 2021

In com-lihaoyi/Ammonite#1229, the main method was renamed. Running the most recent version of ammonite via coursier gives:

Error: Main method not found in class ammonite.Main, please define the main method as:
   public static void main(String[] args)
or a JavaFX application class must extend javafx.application.Application

This can be worked around locally by running:
cs launch ammonite -M ammonite.AmmoniteMain -- yourScript.sc

In com-lihaoyi/Ammonite#1229, the main method was renamed. Running the most recent version of ammonite gives:

```
Error: Main method not found in class ammonite.Main, please define the main method as:
   public static void main(String[] args)
or a JavaFX application class must extend javafx.application.Application
```

This can be worked around locally by running:
`cs launch ammonite -M ammonite.AmmoniteMain -- yourScript.sc`
Copy link
Collaborator

@ckipp01 ckipp01 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 this! Just tested it and and it seems to do the trick. @alexarchambault is it alright to merge this in and do a release? Since Ammonite is currently broken.

@i10416
Copy link

i10416 commented Dec 25, 2021

I think this will break if you specify 2.4.1 or former version.

reproduction

  1. Place ammonite.json in a directory patch
  2. cs install --default-channels=false --channel patch ammonite:2.5.0 succeeds.
  3. cs install --default-channels=false --channel patch ammonite:2.4.1 or former fails.

See https://get-coursier.io/docs/cli-install#create-an-application-descriptor

@blandflakes
Copy link
Contributor Author

I think that will necessarily be true - the former versions specified a different main class. Without a change like this, any version greater than 2.4.1 will fail... is the descriptor a way to set e.g. a minimum version for this change? The docs don't make it obvious to me how to avoid this.

@i10416
Copy link

i10416 commented Dec 25, 2021

I think that will necessarily be true

I don't think so because main class lookup does not work as mentioned in the doc for now.

Specifies an explicit main class to start. Add a ? suffix to use the specified main class only if none is found in the application JAR manifest. (from doc)

MANIFEST.MF
From 2.5.0

Manifest-Version: 1.0
Created-By: Scala mill
Main-Class: ammonite.AmmoniteMain

From 2.4.1

Manifest-Version: 1.0
Created-By: Scala mill
Main-Class: ammonite.Main

But coursier does not seem to use these information.

Update
looking up main class from Jar containing multiple mainClasses depends on Implementation-Vendor-Id and Specification-Title properties on MANIFEST.MF.

This change would work only if ammonite jar contained MANIFEST.MF with Implementation-Vendor-Id and Specification-Title and it does not.
FYI, This PR seems to add theose properties to MANIFEST.MF by mill. Thus, future ammonite release (and replacing older MANIFEST.MF) will fix this issue.

https://github.com/com-lihaoyi/mill/pull/1451/files#diff-d1762355d5afb0548f24242fdbc7f6e4b7fb5653ffc5887fb47641cc450d4270R154

Detail

MainClass.retainedMainClass searches for (org.name,appname),mainClassName here if multiply main classes are found and importantly these (org.name ,appname) are taken from Implementation-Vendor-Id and Specification-Title in META-INF/MANIFEST.MF here in order to distinguish proper mainClass from the rest.

Verbose log says found 4 main classes for ammonite 2.4.1

`Found 4 main classes:
  ((orgname,appname),ammonite.Main)  // <- this line is taken from METAINF/MANIFEST.MF
  ((,Javassist),javassist.CtClass)
  ((,Java Native Access (JNA)),com.sun.jna.Native)
  ((,),ammonite.Main)`

METAINF/MANIFEST.MF generated by mill lacking these properties.

After editting META-INF/MANIFEST.MF like below, I got the log below.

Manifest-Version: 1.0
Created-By: Scala mill
Main-Class: ammonite.Main
+ Specification-Title: ammonite
+ Implementation-Vendor-Id: com.lihaoyi
Found 4 main classes:
  ((com.lihaoyi,ammonite),ammonite.Main) // <- this is taken from METAINF/MANIFEST.MF
  ((,Javassist),javassist.CtClass)
  ((,Java Native Access (JNA)),com.sun.jna.Native)
  ((,),ammonite.Main)

Then, I could install and use ammonite both 2.4.1 and 2.5.0 with this file as ammonite.Main is found in 2.4.1 or former and ammonite.AmmoniteMain is used as fallback in 2.5.0. Thus, change would work only if ammonite jar contained MANIFEST.MF with Implementation-Vendor-Id and Specification-Title.

both cs install --default-channels=false --channel patch ammonite:2.5.0 and cs install --default-channels=false --channel patch ammonite:2.4.1 work.

{
  "mainClass":"ammonite.AmmoniteMain?",
  "repositories": [
    "central"
  ],
  "dependencies": [
    "com.lihaoyi:::ammonite:latest.release"
  ],
  "classifiers": ["_", "sources"],
  "name": "amm"
}

https://github.com/coursier/coursier/blob/990535b8f1fb961a8d54ed668daf65b9c5f35510/modules/install/src/main/scala/coursier/install/InstallDir.scala#L300

@i10416
Copy link

i10416 commented Dec 25, 2021

I think that will necessarily be true - the former versions specified a different main class. Without a change like this, any version greater than 2.4.1 will fail

It seems that channel json file is per app, not per app and version. Therefore, if you set main class ammonite.main, then it will fail for 2.5.0 and if you set ammonite.AmmoniteMain, then it will fail for 2.4.1 and former for now.

The docs don't make it obvious to me how to avoid this.

Yes, I agree. Perhaps, descripter does not assume main class changes over versions.

@i10416
Copy link

i10416 commented Dec 25, 2021

I met similar main class not found issue and that was due to manifest file generated by mill lacking some props like Implementation-Vendor-Id and this may be the culprit.

https://github.com/coursier/coursier/blob/8882fb4f10fac60e2426a7efdc8d29fe6aea2ae5/modules/install/src/main/scala/coursier/install/MainClass.scala#L41

@alexarchambault alexarchambault merged commit 0d52fad into coursier:master Feb 1, 2022
@alexarchambault
Copy link
Member

Thanks @blandflakes, that's merged! (I pushed an extra commit to have things keep working with former Ammonite versions too.)

@blandflakes
Copy link
Contributor Author

@alexarchambault Awesome, thanks for showing how to match those earlier versions!

@blandflakes blandflakes deleted the patch-1 branch February 1, 2022 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants