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

universal script generation for assembly, launcher, and release that run on linux and windows #264

Merged
merged 8 commits into from
Mar 30, 2018

Conversation

lhns
Copy link
Contributor

@lhns lhns commented Mar 28, 2018

As a follow-up to com-lihaoyi/Ammonite#781 this pr generates scripts that can run on linux shell and windows cmd.
It merges the shell and batch scripts introduced in #243.
Feedback is always welcome!

@robby-phd
Copy link
Contributor

@LolHens Thanks for following up on this. Not sure if you are done, but I gave it a spin.

I (purposely) tried running the generated mill.bat on macOS, and it gives me:

Error: Could not find or load main class mill.Main
Caused by: java.lang.ClassNotFoundException: mill.Main

I also tried running the generated mill on cmd, and it gives me:

'#!' is not recognized as an internal or external command,
operable program or batch file.
Error: Could not find or load main class mill.Main
Caused by: java.lang.ClassNotFoundException: mill.Main

So it seems the approach that you used for com-lihaoyi/Ammonite#781 is more robust.

Since the script is supposed to be universal for both sh and cmd, can you also remove releaseAll and releaseBatch and change the call to upload in uploadToGithub to use release instead (i.e., similar to before #243)? That way, only mill gets generated (without mill.bat).

Also, can you make shebang defaults to false? We can change the curl/chmod instructions to add the shebang line.

@lhns
Copy link
Contributor Author

lhns commented Mar 29, 2018

The error you are running into indeed seems very strange. I ran the version I just pushed on my windows system and on a redhat linux machine and it worked fine.
I had to change the approach because before I was prepending a "::" to every line of the shell script so that the cmd skips them however this does not work at all for switch case statements. The new approach tells the cmd to just jump over the shell script and should work for any script.
I removed the releaseAll and releaseBatch and now uploadToGithub only generates "mill".
Also I set the shebang line to be off by default.
Now I am just waiting to see what the tests will say :)

@lhns
Copy link
Contributor Author

lhns commented Mar 29, 2018

I don't really know why the build failed.

@robby-phd
Copy link
Contributor

Can you try running ci/test-mill-release.sh on your redhat machine using mill 0.1.7-8-b913c6? I got some errors (though different than the ones in travis, which looks spurious).

@lhns
Copy link
Contributor Author

lhns commented Mar 29, 2018

Unfortunately I don't have access to that machine right now so I ran the tests on a local debian installation but I couldn't get all of the tests to succeed there.
I made a new commit but changed nothing just to restart the build and it seems to have worked where it previously failed at publishVersion in build.sc:360.
I think there might be some other bug involved that is not related to this pr.
Could you try again to execute the generated scripts?

@robby-phd
Copy link
Contributor

I just tried it again locally on macOS and linux, and tests passed (didn't clean out ~/.mill before). The changes look good to me, thanks.

@robby-phd robby-phd merged commit 107994f into com-lihaoyi:master Mar 30, 2018
@lhns
Copy link
Contributor Author

lhns commented Mar 30, 2018

Very nice, thanks! Should I make a pr to change the generated script in ammonite according to this so that the code is equivalent?

@robby-phd
Copy link
Contributor

That would be great!

robby-phd pushed a commit to com-lihaoyi/Ammonite that referenced this pull request Mar 31, 2018
* changed the method used to skip the script for the other platform to be in sync with com-lihaoyi/mill#264

* set shebang parameter to be false by default

* updated docs on how to prepend the shebang line on linux and added info on how to download on windows
@lihaoyi
Copy link
Member

lihaoyi commented Apr 7, 2018

@robby-phd it seems as of this commit we stopped uploading windows bat files to Releases. Any idea what happened?

@robby-phd
Copy link
Contributor

@lihaoyi The published mill is now both a batch and sh script (universal) so we don't need to publish separate mill releases.

Windows users only need to save mill releases as mill.bat. I have documented this in the master changelog (and in #234 for 0.1.8 release).

@lihaoyi
Copy link
Member

lihaoyi commented Apr 9, 2018

Oh ok, got it

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.

4 participants