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

feat(compilation): compilation directory depends on more cli flags #270

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Samy-33
Copy link
Contributor

@Samy-33 Samy-33 commented Feb 16, 2025

on optimization level, include dirs and the macro defines

Related to #139

➜  compiler+runtime git:(feat/binary-cache-dir) ✗ rm -rf ~/.cache/jank/*
zsh: sure you want to delete all 4 files in /home/saket/.cache/jank [yn]? y
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ ls ~/.cache/jank
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ cat dev/src/a.jank
───────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: dev/src/a.jank
───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ (ns a
   2   │    (:require b))
   34   │  (println "Loading a")
   56   │  (defmacro load-with-delay [& body]
   7   │    (println "Waiting for 2000ms")
   8   │    (clojure.core-native/sleep 2000)
   9   │    `(do ~@body))
  1011   │  (load-with-delay
  12   │    (println "Loading with dealy")
  13   │    (b/hello))
  1415   │  (println "Loaded a")
───────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ cat dev/src/b.jank
───────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: dev/src/b.jank
───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ (ns b)
   23   │ (defn hello [] (println "Hello, world!"))
───────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ time ./build/jank -O 2 --module-path ./dev/src compile a
Bottom of clojure.core
Loading a
Waiting for 2000ms
Loading with dealy
Hello, world!
Loaded a
./build/jank -O 2 --module-path ./dev/src compile a  0.24s user 0.04s system 11% cpu 2.274 total
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ time ./build/jank -O 2 --module-path ./dev/src compile a
Bottom of clojure.core
Loading a
Loading with dealy
Hello, world!
Loaded a
./build/jank -O 2 --module-path ./dev/src compile a  0.49s user 0.04s system 99% cpu 0.530 total
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ ls ~/.cache/jank
x86_64-pc-linux-gnu-7447f407ffb4bafe59ff60f5a258966c3c737e30c91c43ce817d4d17fad2a15f
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ ls ~/.cache/jank/x86_64-pc-linux-gnu-7447f407ffb4bafe59ff60f5a258966c3c737e30c91c43ce817d4d17fad2a15f
classes
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ ls ~/.cache/jank/x86_64-pc-linux-gnu-7447f407ffb4bafe59ff60f5a258966c3c737e30c91c43ce817d4d17fad2a15f/classes
a.o  b.o
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ time ./build/jank -O 1 --module-path ./dev/src compile a
Bottom of clojure.core
Loading a
Waiting for 2000ms
Loading with dealy
Hello, world!
Loaded a
./build/jank -O 1 --module-path ./dev/src compile a  0.38s user 0.04s system 17% cpu 2.418 total
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ tree ~/.cache/jank
/home/saket/.cache/jank
├── x86_64-pc-linux-gnu-7447f407ffb4bafe59ff60f5a258966c3c737e30c91c43ce817d4d17fad2a15f
│   └── classes
│       ├── a.o
│       └── b.o
└── x86_64-pc-linux-gnu-cbbfd6ecd45e15eb3c16b9c59f1c5ebed2d457381af01a6a2f08271b114084a8
    └── classes
        ├── a.o
        └── b.o

5 directories, 4 files
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ time ./build/jank -O 1 --module-path ./dev/src compile a
Bottom of clojure.core
Loading a
Loading with dealy
Hello, world!
Loaded a
./build/jank -O 1 --module-path ./dev/src compile a  0.39s user 0.03s system 99% cpu 0.430 total
➜  compiler+runtime git:(feat/binary-cache-dir) ✗ time ./build/jank -O 1 --module-path ./dev/src compile a
Bottom of clojure.core
Loading a
Loading with dealy
Hello, world!
Loaded a
./build/jank -O 1 --module-path ./dev/src compile a  0.28s user 0.04s system 99% cpu 0.317 total

on optimization level, include dirs and the macro defines
@Samy-33 Samy-33 requested a review from jeaye February 16, 2025 06:58
@Samy-33 Samy-33 marked this pull request as draft February 16, 2025 06:59
Copy link
Member

@jeaye jeaye left a comment

Choose a reason for hiding this comment

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

I can see why you were asking me questions about these two directories. Everything we have in there is confusing. 😂

This is what we want, going forward:

  1. Compiled object files ALWAYS are written to the binary cache dir; there is no option to change that
  2. The CLI option is for changing where the final executable is stored; I think we should just rename this to -o <file> for the CLI command which compiles binaries. We don't need to specify a dir or anything.

I think that means we can rename output_dir in RT ctx to binary_cache_dir, to be super clear of the intention. Then we can remove the --output-dir flag altogether. Later, when we support compiling executables, we can add in the -o <file> flag for that command.

@Samy-33
Copy link
Contributor Author

Samy-33 commented Feb 17, 2025

That makes sense. Now that we do not have output_dir in the runtime context and we don't let users change the binary_cache_dir, we do not need the compilation_path option in command line. So, I have removed it.

@Samy-33 Samy-33 marked this pull request as ready for review February 17, 2025 03:27
@Samy-33 Samy-33 requested a review from jeaye February 17, 2025 03:27
, binary_cache_dir{ fmt::format(
"{}/{}",
util::binary_cache_dir(opts.optimization_level, opts.include_dirs, opts.define_macros),
"classes") }
Copy link
Member

Choose a reason for hiding this comment

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

No need for classes at all. That's a carry-over from the JVM.

Comment on lines -72 to +78
return res = fmt::format("{}/{}", user_cache_dir(), binary_version());
return res = fmt::format("{}/{}",
user_cache_dir(),
binary_version(optimization_level, includes, defines));
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I was wrong, Saket. This is not going to work. If two separate projects both have ns a, with different code, then compiling one project first, then going to compile the other project could result in loading the first project's a when you go to compile the second.

This could work fine for dependencies from maven, but we don't have a good way to distinguish them from project sources right now. To mitigate this, let's put the binary cache dir in target/<binary version>, relative to the current directory. This is what leiningen does. So we should just be able to replace user_cache_dir here with "target".

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.

2 participants