-
Notifications
You must be signed in to change notification settings - Fork 197
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
[Obsolete] Add Android Export feature #545
base: master
Are you sure you want to change the base?
Conversation
- exportAndroid handler which initializes new Cordova project Add a new handler, `exportAndroid`. This takes the source as a parameter, and performs the same steps that the compile route performs, and calls `compileIfNeeded`. After compilation, the `initCordovaProject` function is invoked, which merely creates a stub cordova project, by making a subprocess call to the `cordova` executable. At this commit, it does not build the project.
1ed764a
to
0ad0fd7
Compare
- Create `android-resources` directory that holds app assets - Initialize `android-template` for using as a base for app builds - Handler copies compiled `js` files into template and invokes `cordova` A template directory is initialized with `cordova create` and assets are copied in from `android-resources`. Includes main view HTML, CSS, and App Icons. The handler clones the template directory to `data/<mode>/android/` and copies in the `js` files from the compilation step. `cordova build` is invoked and the generated `.apk` file is fetched as a binary blob and downloaded.
- Add Cordova and JDK installation to install.sh - Build Android template project building - Set android build tools paths in base.sh List of Dependencies for Android Exports - - Cordova >= 6.2.2 - Android Build Tools - Android SDK >= 19 - Gradle >= 3.4 - All executables in path and $ANROID_HOME set
- Add XML Parser TagSoup to set the App Name in `config.xml` - Add UI components to receive App Name input
Hi Venkat. Thanks for the pull request. I have some concerns about the performance implications of this change, and there are some critical times coming up (CodeWorld is being used for an ICFP presentation in a few hours, and at Chalmers through this coming Tuesday. Since this adds the possibility of an expensive server-side build process, it's best not to merge this until after those milestones have passed. I'll take a look soon, though, and see if there are any major concerns. |
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.
Sorry for the delay in reviewing this. The end of Summer of Haskell left me swamped with work to do.
<access origin="*" /> | ||
<allow-intent href="http://*/*" /> | ||
<allow-intent href="https://*/*" /> | ||
<allow-intent href="tel:*" /> |
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.
What is the purpose of all these allow-intent elements?
CodeWorld App | ||
</description> | ||
<author href="https://code.world"> | ||
CodeWorld |
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.
Maybe set this to "CodeWorld User" so it doesn't seem like we're claiming authorship of student programs?
<widget id="io.cordova.hellocordova" version="1.0.0" xmlns="http://www.w3.org/ns/widgets" xmlns:cdv="http://cordova.apache.org/ns/1.0"> | ||
<name>CodeWorld App</name> | ||
<description> | ||
CodeWorld App |
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 could be more descriptive, right? Maybe "This application is exported from CodeWorld." or something?
@@ -0,0 +1,27 @@ | |||
<?xml version='1.0' encoding='utf-8'?> | |||
<widget id="io.cordova.hellocordova" version="1.0.0" xmlns="http://www.w3.org/ns/widgets" xmlns:cdv="http://cordova.apache.org/ns/1.0"> |
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.
io.cordova.hellocordova ???
Would there be a problem installing two of these? Maybe the id would need to have the program hash in it to disambiguate? But I'm not sure, because I'm not sure what this is used for.
@@ -0,0 +1,105 @@ | |||
* { |
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.
Please add a source header, which you can copy from other css files in the project (e.g., in web/css).
mode <- getBuildMode | ||
Just source <- getParam "source" | ||
maybeAppName <- getParam "appName" | ||
let appName = BC.unpack $ fromMaybe (BC.pack "CodeWorld App") maybeAppName |
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.
Is there ever a case you expect to get a request without an app name? If not, just fail the pattern match in that case.
B.writeFile (buildRootDir mode </> sourceFile programId) source | ||
writeDeployLink mode deployId programId | ||
compileIfNeeded mode programId | ||
unless success $ modifyResponse $ setResponseCode 500 |
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.
If you get a 500 error, you shouldn't continue, right?
writeDeployLink mode deployId programId | ||
compileIfNeeded mode programId | ||
unless success $ modifyResponse $ setResponseCode 500 | ||
let appProps = M.fromList [("appName", appName)] |
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 AppProps thing is entirely unnecessary. You're trying to invent a dynamically typed language in Haskell, and it's ugly. Just go ahead and commit to defining a data type with the right fields. (Which, at the moment, is just one String, right?)
androidBuildDir mode programId = androidRootDir mode </> sourceBase programId | ||
|
||
apkFile :: BuildMode -> ProgramId -> FilePath | ||
apkFile mode programId = |
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.
Once you move the build into a temporary directory (see other comment), you will want to pick a simpler path for the apk file.
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.
Since you're letting people choose different app names for the same code, you cannot uniquely identify apk files by the hash of their code. You need a different scheme.
nameId = (~/= ("<name>"::String)) | ||
|
||
buildApk :: BuildMode -> ProgramId -> IO () | ||
buildApk mode programId = do |
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.
You need to set a time limit, and kill the process if it exceeds the time limit. There's a utility for that at https://github.com/google/codeworld/blob/master/codeworld-compiler/src/Compile.hs#L203, but unfortunately it was moved from codeworld-server to codeworld-compiler, as part of another Summer of Haskell project. For now, feel free to copy it back into codeworld-server/src/Util.hs
Apologies for the delay! I've been having tests this week, I'll get on this immediately. |
exportAndroid
handler which compiles a.apk
and prompts a download