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

Only compile files defined as source. #231

Closed
wants to merge 0 commits into from
Closed

Conversation

kul
Copy link
Contributor

@kul kul commented Jun 17, 2015

Currently doing an aot will also try to compile data_readers.clj or any other resource file ending with .clj e.g. an edn file. This patch modifies aot and java compilation by limiting file set with :source mask only.

@micha
Copy link
Contributor

micha commented Jun 18, 2015

Thanks for the PR! But won't this make it impossible to AOT Clojure namespaces in :resource-paths?

Tasks that compile or build things should always operate on files with input role. Whether a file is in :source-paths or :resource-paths is not relevant to a build task, because both add the input role. The difference between :source-paths and :resource-paths is that only :resource-paths has the output role. Packaging tasks, unlike build tasks, operate on files with the output role.

With this patch you couldn't, for example, build a JAR with AOT and the Clojure source files in the JAR, because the usual way to include things in packaging is to ensure that they have the output role (i.e. in this case put them in :resource-paths).

<rant> This, incidentally, is an example of how we abuse file extensions. Why not use the .edn extension for EDN files, leaving .clj for Clojure namespaces? It's for this reason that we used build.boot for example, and not build.clj. It takes 2 seconds to associate build.boot with file format for syntax highlighting in your editor, but in return we get useful information from the file extension and our tools can help us do things. </rant>

So in this case the only things we can do are:

  • examine the file and use some heuristic to determine whether it looks like a compilable clojure namespace
  • special-case handling for data_readers.clj

What do you think?

@kul
Copy link
Contributor Author

kul commented Jun 19, 2015

Hi @micha ,
Totally agree with the rant. But I still do not quite agree on the part of compiling resources.

The problem you pointed out of both aot and source packaging is reproducible with the patch, I didnt test it enough. I was under the impression that adding to both resource and source paths would handle this case but it turns out the get-files function works differently and would not return a file for source mask if added to both src and resource.

I started this patch with a filter for data_readers but soon figured its brittleness. No matter what we argue people are at some point going to do weird things, data_readers.clj is a great example of such abuse.

What do you propose?

@micha
Copy link
Contributor

micha commented Jun 19, 2015

@kul I propose one or both of the following:

  • Examine files with .clj extension and use some rules for testing whether the file is a Clojure namespace or not (eg. look for ns macro)
  • Add special handling for known non-namespace .clj files, data_readers.clj being one of them
    • We may want to add options to the aot task similar to what we've done with the uber task: options to allow regex exclusion with sane defaults.

@kul
Copy link
Contributor Author

kul commented Jun 20, 2015

@micha Yes exclusion regex makes total sense. Ok i will have a crack at it and squash in new commits. Thanks!

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

2 participants