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

Total rewrite of the package (aka make PackageCompilerX the new PackageCompiler) #304

Merged
merged 80 commits into from
Feb 10, 2020

Conversation

KristofferC
Copy link
Sponsor Member

@KristofferC KristofferC commented Feb 9, 2020

This PR completely replaces PackageCompiler.jl with that of PackageCompilerX.jl. The maintainers of this package as well as those of PackageCompilerX.jl feels like this is the best way forward.
A few options were considered:

  • Just deprecate this repo, rename PackageCompilerX to something else (AppCompiler.jl). There are some drawbacks to this. Firstly, this package name is already pretty good and it would be a shame to waste it. Secondly, it is a bit confusing to have a lot of references spread out over the internet and then have it be deprecated. Also, it would remove the history of everyone contributing to this package which is a shame.
  • Just repoint the URL in the registry to PackageCompilerX. This would cause existing manifests to break so it is a non-starter.

Instead, just putting a complete rewrite on top of the existing history felt like the best choice.

Some improvements / changes in PackageCompilerX.jl vs the current PackageCompiler:

  • Support for artifacts in Apps. This allows one to distribute apps that depend on e.g. libraries via the artifact system to other machines.
  • More CI, including e.g. ARM support.
  • Better defaults for cpu targets, matching those that Julia itself uses
  • Proper documentation including some examples, tutorials and developer docs.
  • No command line interface, this felt like adding a lot of code for little benefit.
  • Uses the Artifact system to get a compiler for Windows instead of the largely deprecated WinRPM.
  • Drops pre 1.3.1 Julia support.
  • There is no need to annotate the entry points of apps with Base.@ccallable since that is done automatically.
  • The julia_main function no longer take any arguments. Just like in normal Julia code, just access the global ARGS.
  • An app needs to be structured as a package. (project file and src/Package.jl file)

I will go through earlier issues and add here which are either fixed by this PR, no longer relevant due to fixes in Julia or changes in PackageCompiler functionality:

Closes #52
Closes #83
Closes #108
Closes #129
Closes #131
Closes #289
Closes #297
Closes #290
Closes #281
Closes #192
Closes #277
Closes #276
Closes #272
Closes #271
Closes #270
Closes #258
Closes #255
Closes #252
Closes #234
Closes #233
Closes #230
Closes #236
Closes #228
Closes #225
Closes #223
Closes #222
Closes #221
Closes #220
Closes #215
Closes #212
Closes #208
Closes #202
Closes #201
Closes #194
Closes #193
Closes #190
Closes #184
Closes #223
Closes #182
Closes #180
Closes #163
Closes #151
Closes #155
Closes #154
Closes #152
Closes #145
Closes #141
Closes #226
Closes #140
Closes #138
Closes #117
Closes #76
Closes #59

Got kinda lost with isues at this point. Will take another pass after this PR is merged.

After this PR is mergd, we should transfer over issues PR from PackgeCompilerX to this repo.

KristofferC and others added 30 commits February 9, 2020 14:42
* implement support for precompile file
* wip only using needed stdlibs

* only include needed stdlibs in apps

* make default off
Don't add an rpath to local absolute path
* improvements to cpu targets

* more

* more

* remove precompile from testing due to JuliaLang/julia#34076
* wip make app work on windows

* wip win

* test artifact load path

* realpath
@SimonDanisch
Copy link
Collaborator

Yay :)

@DilumAluthge
Copy link
Member

@KristofferC Are you planning on making a release as soon as you merge this PR?

If not, it might be better to set the version to 1.0.0-DEV.

@SimonDanisch
Copy link
Collaborator

Awesome :) Do we want to try to fix the CI, or is that not really doable, since tests take so long? Could we split up stuff?

@SimonDanisch
Copy link
Collaborator

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

I guess this time out can be upped?

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Feb 10, 2020

Strange, it passed on ARM in the previous commit and it took like 25 min. Maybe it was barely though.

* JuliaComputing -> JuliaLang

* Build documentation on correct conditions

push to master and pull request based on master
@DilumAluthge
Copy link
Member

Travis is passing.

It looks like the AppVeyor error isn’t actually a test failure, but a problem with the configuration of AppVeyor on this repository.

How about we uninstall AppVeyor from this repo and use Travis for the Windows tests? After all, this PR deletes the .appveyor.yml file, which implies that we aren’t planning on using AppVeyor.

I think you need to be an organization admin of the JuliaLang organization in order to uninstall the AppVeyor app from this repo. @ViralBShah @StefanKarpinski

LICENSE Outdated
@@ -0,0 +1,19 @@
Copyright (c) 2019 Kristoffer Carlsson <kristoffer.carlsson@juliacomputing.com>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I mean I see that this code is mainly by you, but would it make sense to leave me in here, or do we just say, its in the history?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like: Kristoffer, Simon, Luca, and other contributors? Just to be comprehensive.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds fair!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would appreciate that, as I put a lot of effort on PackageCompiler.jl development.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely ;) That I was alone in the license was wrong to begin with!

Copy link
Member

Choose a reason for hiding this comment

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

Can someone make a suggestion here so it can just be accepted? Otherwise, we absolutely must add the names. Also let's add Julia Computing to the mix please.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'll make a suggestion

Copy link
Sponsor Member Author

@KristofferC KristofferC Feb 10, 2020

Choose a reason for hiding this comment

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

Sorry, didn't think about this, it was just the default that got generated by PkgTemplates. Let's merge all names in here :)

@ViralBShah
Copy link
Member

I deleted the appveyor webhook.

LICENSE Outdated Show resolved Hide resolved
Co-Authored-By: Dilum Aluthge <dilum@aluthge.com>
@NHDaly
Copy link
Member

NHDaly commented Feb 13, 2020

Hooray! Thanks for the hard work on this, everyone! 😁 The future is bright!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment