WIP - Add lumo script for building the lambda zip #6

arichiardi · 2017-08-18T18:12:22Z

Hi! 😄 here I am with the script 🎉

Path is maybe off for a script, I just wanted to put it out there.

Thanks to this I am able to compile (cljs bootstrap compatible) lambdas. I am now missing the deploy part and that is why I am going to ask some help here 😄

I also have a question about the serverless.yml file itself, because maybe we could define custom entries for bootstrapped compilation (which requires extra care), for instance:

 handler-lambda-cljs:
    cljs-lumo: handler.core/my-lambda

Open to thoughts - suggestions - corrections!

mattbishop · 2017-08-18T18:36:07Z

serverless-cljs-plugin/js_build.cljs

+  ;; TODO serverless-opts?
+  (let [{:keys [compiler functions resource-dirs env]} cljs-lambda-opts
+        {:keys [inputs options]} compiler
+        ;; recurring :keys don't word in a let


word -> work?

mattbishop · 2017-08-18T18:37:31Z

serverless-cljs-plugin/js_build.cljs

+  )
+
+
+(comment


Should this be in an examples.cljs file?

moea · 2017-08-18T18:52:13Z

@arichiardi I appreciate this - I'm going to have to learn a little about lumo before merging - a couple of high-level things maybe you can help with:

The build function accepts a leiningen-formatted project map - do lumo projects typically have those? If they do, is there a problem with reading it from the project directory in the module's main function?
serverless-cljs-plugin passes the output zip path to cljs-lambda - this script should also accept it, and only ever write the zip there.
Why does the script expect there to be an index.js on the filesystem? Maintaining it will be a pain for the users - we should generate it for them, and add it to the zip file. I can do that, if you're not comfortable with it.

To keep it simple, let's keep serverless.yml the same, and have the 'use-lumo' thing be a switch accepted by serverless-cljs-plugin. Either all cljs functions are lumo, or all are jvm.

arichiardi · 2017-08-18T19:22:48Z

Lumo is like the vanilla cljs compiler, so you pass things in. I will probably then need to read the project.clj file from the script.

The output of the zip can be passed to the zip! function. Now it is hardcoded to .serverless

About index.js, I thought it was a convention coming from the cljs-lambda template but you are right, somebody could choose not to use that.

moea · 2017-08-18T19:31:06Z

@arichiardi - in order:

If that's the case, then there's not likely to be a project.clj, right? We need to either establish a convention of storing an edn file in the project directory with the compiler settings in them, or allow to them to be specified in yaml and placed in serverless.yml. Probably the second one.
I don't just mean the containing directory, I mean the whole path - this script doesn't need to guess names. This is important because the serverless plugin needs to know where the zip file is once the script is done.
It's the convention because cljs-lambda generates the file - it's an internal convention. cljs-lambda doesn't even support user-maintained index.js files, and there hasn't been a need to. So generating the index file with this script is definitely the way to do.

moea · 2017-08-18T20:11:58Z

@arichiardi This is a great idea!

arichiardi · 2017-08-18T20:30:42Z

what information we would need from project.clj? Lumo projects usually work directly from the node_modules folder.
I saw in the jvm one you return the path of the zip, are you referring to this? The zip file name can be passed in btw, probably it would be good to establish what is the input to a build, I will investigate this as well
so what do you suggest I should do for this then? I will change according to your suggestions.

Just to give more context, this has to be executed with:

lumo serverless-cljs-lambda/js_build.cljs param1 param2

So we can pass things from the command line as well as reading project.clj directly

moea · 2017-08-18T21:02:20Z

The compiler arguments. I'm not saying we need project.clj, just that since we don't have it, we need another convention for specifying compiler arguments. Ideally serverless-cljs-plugin would support something like this in serverless.yml:

custom:
  lumo:
    optimizations: none
    sourceMap: true

Or whatever is applicable to lumo. We should default everything, and make up our own output-to, output-dir, etc. inside the script - it doesn't make sense for the user to specify those.

It returns the path to the zip, but that doesn't get used - it also optionally takes a path to a zip, which the serverless plugin always passes. Currently i'm imagining the inputs would be "edn string of compiler arguments" and "output zip path"
I can do that bit, it'll be very similar to what the jvm plugin does. The script will have to take a list of the functions that were parsed out of serverless.yml, so it knows what handlers to generate - I can take care of that also.

arichiardi · 2017-08-18T21:10:22Z

Ok so basically what you'd need is to modify the build! function, it seems all the parameters you describe are already input to compile! and zip! at the moment. One of the nice conventions that I have seen more and more around is to have edn files. So we could baptize here our cljs-lambda.edn which will contain compiler options, inputs..same as the :cljs-lambda that at the moment is nested in project.clj. How does it sound? Also, are you going to spawn a new branch or you want to continue on this one? Just so I don't rebase it. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

moea · 2017-08-18T21:20:33Z

That's fine if you prefer that, it ends up being more or less the same thing - as long as the file is optional, and the script uses sensible defaults both for values unspecified in the file, and when the file does not exist. Do you want to implement that bit?

I'll branch from these changes.

arichiardi · 2017-08-18T21:27:49Z

Well it is definitely easier to start from edn, then once the feature is ready we can think of parsing yaml (which is definitely more familiar for JS users, but is always limited)... I guess that yes I can hook the edn.

The plugin in the feature if necessary could in order scan:

yaml section
cljs-lambda.edn
project.clj

But ok step by step. In any case cool stuff!

arichiardi · 2017-08-22T14:00:41Z

A question, should I keep the structure of the current cljs-lambda config map? I guess it would be a bit easier for your users to understand and for your to wrap your head around.

For me, I like something like:

{:project {:name "logpoc" 
           :source-paths ["src"] 
           :target-path "target"
           :resource-paths ["resources"]}
 :compiler {:options {:target        :nodejs
                      :language-in   :ecmascript5
                      :optimizations :simple
                      :main "logpoc.core"}}
 :functions [{:name "logpoc-dev-bunyan-lambda"
              :invoke logpoc.core/bunyan-lambda}]}

Where :inputs has become a project property and the naming follows the ones in boot and lein.

What do you think?

Ps.: For the generation there is either https://github.com/fotoetienne/cljstache or https://github.com/janl/mustache.js.

arichiardi · 2017-08-22T17:55:56Z

index.js

@@ -54,6 +54,9 @@ function cljsLambdaBuild(serverless, opts) {
  const cmd = (`lein update-in :cljs-lambda assoc :functions '${fns}' ` +
               `-- cljs-lambda build :output ${serverless.service.__cljsArtifact} ` +
               `:quiet`);
+  // TODO if it is a lumo function use our lumo script for compiling
+  // const cmd = (`lumo -c serverless-cljs-plugin -m lumo.build` +


This is the command that works, but you need lumo on master for it to work, I am patching lumo, see anmonteiro/lumo#237

arichiardi · 2017-08-22T17:56:53Z

serverless-cljs-plugin/lumo/build.cljs

+  It is very similar to:
+
+    https://github.com/nervous-systems/cljs-lambda/wiki/Plugin-Reference#exhaustive-config-map"
+  [zip-path cljs-lambda-opts]


Now we pass the path of the zip (output) and the options

arichiardi · 2017-08-22T17:57:48Z

serverless-cljs-plugin/lumo/build.cljs

+        conf (read-conf!)]
+    (if-not (instance? js/Error opts-or-err)
+      (do (.info js/console "Building" (get-in conf [:project :name]))
+          (build! (:z opts-or-err) (read-conf!)))


The options are read from cljs-lambda.edn but read-conf! can be augmented in order to handle reading from anywhere basically.

serverless-lumo.edn might be a more appropriate name?

Yes I like it.

arichiardi · 2017-08-22T17:58:13Z

serverless-cljs-plugin/lumo/build.cljs

+(defn build!
+  "Build a project.
+
+  The cljs-lambda-opts map is:


The shape of the map of course is debatable

moea · 2017-08-22T19:42:58Z

serverless-cljs-plugin/lumo/build.cljs

+
+(defn compiler-defaults
+  "Generate the compiler options map part for the output."
+  [target-path project-name]


I think we should remove references to project-name - here and in the function at the top of the module - as far the args here, we should consider just defaulting to {:output-to "out/lambda.js" :output-dir "out" :target nodejs} or something simple like that. What about defaulting optimisation level?

Well :optimizations might be good to have configurable, for dead code elimination. I would not want to restrict that.

About the output, I am still torn, I like the way normal serveless does it, prepending even the stage in front of the name of the .js file. So I think at some point we'll need to configure it...

Also, there is another issue, if we have output-dir we force to split the files...otherwise one .js file will be generated. No sure here, but can the split impact performance?

Regarding optimisations - these are defaults, right? They should all be trivial to override if the user supplies alternate values in the config, but we should have sensible, well defined defaults - like optimizations: none

Does that happen even with optimizations :none - you get a single file if you don't specify output-dir? In any case, I'm fine with separate files - advanced compilation will yield a single file, so the user can specify that if they want.

If I remember correctly it always happens, in any case the defaults now are:

{:output-to "out/lambda.js", :output-dir "out", :target :nodejs, :optimizations :none}

moea · 2017-08-22T19:44:31Z

yarn.lock

@@ -0,0 +1,241 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


I'm not familiar with this format. Do we actually need this?

oh sorry, the yarn.lock is generated by yarn and usually committed, I though we were already using it, will remove.

moea · 2017-08-22T19:46:23Z

@arichiardi I added some superficial commentary - when you're ready, if you reopen the pull request against the branch wip/lumo, I can make the index changes there, and will discuss with you before merging into master.

arichiardi · 2017-08-22T19:50:04Z

I am actually thinking about a change that will pass --functions at the comment line, new call:

lumo -c scripts -m lumo.build --zip-path .serverless/logpoc.js --functions ....

What do you think?

I see that functions is computed in index.js...so it cannot go in cljs-lambda.edn option.

moea · 2017-08-22T20:40:34Z

That's what we need, yeah - it's edn, so e.g. --functions '[{:name ...} ...]' - see index.js for specifics.

arichiardi · 2017-08-22T21:15:31Z

Ok so this last version works fine with:

lumo -c scripts -m lumo.build --zip-path .serverless/logpoc.js --functions "[{:name logpoc :invoke logpoc}]"

Quotes are necessary. I will push the changes to wip/lumo if there are not objections.

arichiardi · 2017-08-22T21:22:26Z

Ok @moea no objections but I need to be Collaborator in order to push to wip/lumo 😄

arichiardi · 2017-08-22T21:25:46Z

Btw thanks to this the lumo command will be greatly simplified.

moea · 2017-08-22T21:32:31Z

@arichiardi I can add you as a collaborator, but can't you just open a PR against that branch and I can merge it?

arichiardi · 2017-08-22T21:36:49Z

Oh, never tried that, I thought you can only merge to master from github unless you change the default branch. Are you sure it is going to work?

moea · 2017-08-22T21:38:06Z

https://github.com/blog/2224-change-the-base-branch-of-a-pull-request

arichiardi · 2017-08-22T21:39:24Z

Wow awesome did not know that :)

moea · 2017-08-22T21:40:13Z

@arichiardi I am pressed for time, but hope to have an example with index generation etc. working by the end of the week.

arichiardi · 2017-08-22T21:42:14Z

I will try that too then, I am pressed for time as well ;) Thanks for your hints, this is going to be a very nice feature

moea · 2017-08-22T21:44:15Z

@arichiardi Let's not both do the same thing!

arichiardi · 2017-08-22T21:54:37Z

Ok you are right let me do that then, I need to make this work soon...I was wondering if the local invoke can be worked on as well...for sure you are more familiar with it. What do you think?

moea · 2017-08-22T22:12:05Z

@arichiardi I wouldn't feel comfortable delegating the index generation task, it needs to be as similar as possible to what cljs-lambda is doing, and work for all optimization levels. If you are in a hurry to get it working, just manually write an index file once for your project, and add it to the zip.

I am personally not particularly interested in local invoke - I can just call the exposed function from a REPL. If you are interested in it, and it doesn't involve far reaching changes, then I am happy to include it.

arichiardi · 2017-08-23T06:06:42Z

Ok sounds good.

moea · 2017-08-23T12:53:33Z

@arichiardi If you create a project which depends on serverless-cljs-plugin 0.2.0-alpha, then serverless deploy --lumo or serverless package --lumo should work. I've only tested with optimizations :none. See lumo-example in the branch wip/lumo for a working example.

moea · 2017-08-23T12:55:29Z

Note that I changed the format of the config file and made it optional - see commits in the branch for details.

arichiardi · 2017-08-23T13:24:05Z

Oh you went for a cmd line option for that, I was thinking of having a lumo key instead of cljs in serverless.yml, it is more convenient (less to type and to think about, what if I forget the switch? 😄).
I am checking the commits ;)

moea · 2017-08-23T14:16:32Z

Having lumo instead of cljs would be per-handler, so it would give the mistaken impression that it's possible to mix both cljs and lumo handlers in a single serverless.yml, which it's not.

It would be possible to also support a top level custom lumo: true option, but I don't think it makes a huge difference.

arichiardi · 2017-08-23T15:28:17Z

I like the lumo: true option better - or even cljs-compiler: lumo. I can contribute that, is it going to be in opts in the plugin?

moea · 2017-08-23T16:30:08Z

Yeah, so ideally it should work if you do either this:

custom:
  lumo: true

or deploy with --lumo. I think changing the if condition to if(_.get(serverless.service, 'custom.lumo') || opts.lumo) is sufficient.

* Add lumo script for deploying self-host lambdas (#6) * Simplify option parsing, misc. fixes * Working end-to-end example * Misc improvements to lumo build (#7) * Handle archive warning * Use apply for sequable :files entries in zip! * Make keyword lookup idiomatic * Handle custom.cljsCompiler option in serverless.yml This patch allows to specify the cljs compiler of the build. At the moment only lumo is the alternative to the default JVM compiler (which requires lein), but in the future others can be added (a vanilla cljs script, shadow-cljs, boot-cljs...). * Add Lumo compiler section to README * Work-around lumo's command-line-args bug and support all versions * Use `simple` template for `advanced` compilation * 0.2.0

arichiardi force-pushed the lumo-build branch from 0ec757b to 46276a9 Compare August 18, 2017 18:23

mattbishop reviewed Aug 18, 2017

View reviewed changes

serverless-cljs-plugin/js_build.cljs Outdated

)

(comment

Copy link

mattbishop Aug 18, 2017

Choose a reason for hiding this comment

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

Should this be in an examples.cljs file?

arichiardi force-pushed the lumo-build branch from 46276a9 to 2c99e03 Compare August 22, 2017 17:51

arichiardi commented Aug 22, 2017

View reviewed changes

arichiardi force-pushed the lumo-build branch from 2c99e03 to ac1d57c Compare August 22, 2017 18:38

moea reviewed Aug 22, 2017

View reviewed changes

arichiardi force-pushed the lumo-build branch 2 times, most recently from 5e67969 to 40e4350 Compare August 22, 2017 20:54

Add lumo script for deploying self-host lambdas

0716060

arichiardi force-pushed the lumo-build branch from 40e4350 to 0716060 Compare August 22, 2017 21:10

moea changed the base branch from master to wip/lumo August 22, 2017 21:38

moea merged commit 72b9c86 into nervous-systems:wip/lumo Aug 22, 2017

arichiardi deleted the lumo-build branch August 22, 2017 21:39

arichiardi mentioned this pull request Aug 23, 2017

Consider dropping lein in favor of direct aws cli calls? #4

Closed

		@@ -0,0 +1,241 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

WIP - Add lumo script for building the lambda zip #6

WIP - Add lumo script for building the lambda zip #6

Conversation

arichiardi commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moea commented Aug 18, 2017

arichiardi commented Aug 18, 2017

moea commented Aug 18, 2017 • edited Loading

moea commented Aug 18, 2017

arichiardi commented Aug 18, 2017 • edited Loading

moea commented Aug 18, 2017

arichiardi commented Aug 18, 2017 via email

moea commented Aug 18, 2017

arichiardi commented Aug 18, 2017

arichiardi commented Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moea Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arichiardi Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

moea commented Aug 22, 2017

arichiardi commented Aug 22, 2017 • edited Loading

moea commented Aug 22, 2017

arichiardi commented Aug 22, 2017

arichiardi commented Aug 22, 2017

arichiardi commented Aug 22, 2017

moea commented Aug 22, 2017

arichiardi commented Aug 22, 2017

moea commented Aug 22, 2017

arichiardi commented Aug 22, 2017

moea commented Aug 22, 2017

arichiardi commented Aug 22, 2017

moea commented Aug 22, 2017

arichiardi commented Aug 22, 2017

moea commented Aug 22, 2017

arichiardi commented Aug 23, 2017

moea commented Aug 23, 2017

moea commented Aug 23, 2017

arichiardi commented Aug 23, 2017

moea commented Aug 23, 2017

arichiardi commented Aug 23, 2017 via email

moea commented Aug 23, 2017

moea commented Aug 18, 2017 •

edited

Loading

arichiardi commented Aug 18, 2017 •

edited

Loading

arichiardi commented Aug 22, 2017 •

edited

Loading

moea Aug 22, 2017 •

edited

Loading

arichiardi Aug 22, 2017 •

edited

Loading

arichiardi commented Aug 22, 2017 •

edited

Loading