build: Disable prql-elixir on Mac (#1902)

* build: Disable prql-elixir on Mac

A temporary pause on building `prql-elixir` on Mac, as it's causing some build caching issues, as described in the Readme

There are lots of ways to re-enable when we're ready.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

.cargo/config.toml

@@ -1,9 +1,2 @@
 [target.wasm32-unknown-unknown]
 runner = 'wasm-bindgen-test-runner'
-
-[target.'cfg(target_os = "macos")']
-# Required for prql-elixir on Mac
-rustflags = [
-    "-C", "link-arg=-undefined",
-    "-C", "link-arg=dynamic_lookup",
-]

.github/workflows/test-elixir.yaml

@@ -25,7 +25,8 @@ jobs:
   test:
     strategy:
       matrix:
-        os: [ubuntu-latest, windows-latest, macos-latest]
+        # Currently disabling mac tests, see prql-elixir/readme.md for more details.
+        os: [ubuntu-latest, windows-latest]
         otp: ["25.1.2"]
         elixir: ["1.14.2"]
     runs-on: ${{matrix.os}}

Taskfile.yml

@@ -64,7 +64,7 @@ tasks:
         # Can't install atm with `--locked`
       - cargo install mdbook-footnote
       - cmd: |
-          [ "$(which default-target)" ] || echo "🔴 Can't find a binary that cargo just installed. Is the cargo bin path (generally at ~/.cargo/bin) on the \$PATH?"
+          [ "$(which cargo-insta)" ] || echo "🔴 Can't find a binary that cargo just installed. Is the cargo bin path (generally at ~/.cargo/bin) on the \$PATH?"
         silent: true
 
   install-maturin:
@@ -151,17 +151,9 @@ tasks:
       Build everything.
 
       Running this isn't required when developing; it's for caching or as a reference.
-    vars:
-      default_target:
-        sh: default-target
-      targets: |
-        {{ .default_target }}
-        wasm32-unknown-unknown
     cmds:
-      - |
-        {{ range ( .targets | trim | splitLines ) -}}
-        cargo build --all-targets --target={{.}}
-        {{ end -}}
+      - cargo build --all-targets --target=wasm32-unknown-unknown
+      - cargo build --all-targets
       - task: build-web
 
   test-all:
@@ -177,14 +169,10 @@ tasks:
       - task: test-rust
       - task: build-all
       - task: test-js
-      - task: test-elixir
       - pre-commit run -a
 
   test-rust:
     desc: Test all rust code, accepting snapshots.
-    vars:
-      default_target:
-        sh: default-target
     # Commenting out the ability to watch, since Task seems to trip over itself,
     # starting a new process before the old one has finished when a process
     # changes output files.
@@ -212,13 +200,11 @@ tasks:
       # tests to extract them, which include compiling prql-compiler, c) not
       # fail other compilation steps if they can't be extracted.
       - cargo insta test --accept -p mdbook-prql --test snapshot
-        --target={{.default_target}}
       # Only delete unreferenced snapshots on the default target — lots are
       # excluded under wasm. Note that this will also over-delete on Windows.
       # Note that we need to pass the target explicitly to manage
       # https://github.com/rust-lang/cargo/issues/8899
-      - cargo insta test --accept --target={{.default_target}}
-        --unreferenced=auto
+      - cargo insta test --accept --unreferenced=auto
       - cargo insta test --accept --target=wasm32-unknown-unknown
       # We build the book too, because that acts as a test
       - cd book && mdbook build
@@ -248,7 +234,6 @@ tasks:
       - "prql-compiler/**/*.snap"
     cmds:
       - cargo insta test --accept -p prql-compiler --lib
-        --target=$(default-target)
 
   build-web:
     desc: Build the website, including the book & playground.
@@ -276,14 +261,15 @@ tasks:
       - npm run build
       - npm cit
 
-  test-elixir:
-    dir: prql-elixir
-    cmds:
-      # We could move this line into an `install` task
-      - mix local.hex --force
-      - mix deps.get --force
-      - mix compile
-      - mix test
+  # Currently disabled; see prql-elixir/README.md for details
+  # test-elixir:
+  #   dir: prql-elixir
+  #   cmds:
+  #     # We could move this line into an `install` task
+  #     - mix local.hex --force
+  #     - mix deps.get --force
+  #     - mix compile
+  #     - mix test
 
   run-website:
     desc: Build & serve the static website.

book/book.toml

@@ -13,10 +13,7 @@ git-repository-url = "https://github.com/PRQL/prql"
 # This is required because mdbook-prql isn't necessarily installed; maybe
 # there's a better way.
 #
-# We put the target directory in `target-book` because of
-# https://github.com/rust-lang/cargo/issues/8899, as an alternative to requiring
-# `default-target` to be installed.
-command = "cargo run --bin mdbook-prql --target-dir=target-book"
+command = "cargo run --bin mdbook-prql"
 
 [preprocessor.admonish]
 assets_version = "2.0.0" # do not edit: managed by `mdbook-admonish install`

prql-compiler/Cargo.toml

@@ -40,9 +40,9 @@ insta = {version = "1.28", features = ["colors", "glob", "yaml"]}
 # For integration tests
 [target.'cfg(not(target_family="wasm"))'.dev-dependencies]
 chrono = "0.4"
-pretty_assertions = "1.3.0"
 criterion = "0.4.0"
 postgres = "0.19.3"
+pretty_assertions = "1.3.0"
 rusqlite = {version = "0.28.0", features = ["bundled", "csvtab"]}
 
 # Re-enable on windows when duckdb supports it

prql-elixir/README.md

@@ -38,3 +38,34 @@ crate from this repo:
 
 Future work includes publishing pre-compiled artifacts, so Elixir projects can
 run PRQL without needing a Rust toolchain.
+
+## Mac
+
+We currently don't enable compilation for Mac. This is possible to enable, but
+causes some issues with cargo's compilation cache. Briefly: it requires
+`RUST_FLAGS` to be set, and because of
+<https://github.com/rust-lang/cargo/issues/8716> &
+<https://github.com/rust-lang/cargo/issues/8899>, any compilation of a different
+target will bust the cache.
+
+The possible future workarounds include:
+
+- Passing `--target=aarch64-apple-darwin` to every cargo call, which is
+  inconvenient and can be difficult in some situations; e.g. Rust Analyzer. This
+  disables passing `RUST_FLAGS` (I'm actually unclear why `prql-elixir` builds
+  successfully in that case...)
+- Directing other cargo calls to different paths, such as `/target-ra` for Rust
+  Analyzer and `/target-book` for the book building. But one `cargo build` from
+  the terminal without either the `target` or `target_dir` specified will bust
+  the cache!
+- Never compiling for other targets. But our standard tests run for
+  `--target=wasm32-unknown-unknown`, so this requires refraining from using
+  them.
+- Removing `prql-elixir` from our workspace, so that `cargo` commands in the
+  PRQL workspace don't require rust flags. This would work well, but means we
+  need separate test coverage for this crate, which adds some weight to the
+  tests.
+
+If `prql-elixir` becomes more used (for example, we start publishing to Hex, or
+Mac developers want to work on it), then we can re-enable and deal with the
+caching issues. We can also re-enable them if the `cargo` issue is resolved.

prql-elixir/native/prql/.cargo/config.toml

@@ -1,3 +1,6 @@
+# Note that this doesn't apply when compiling from the workspace root. Because
+# we're not currently compiling for Mac, it's also impotent for now.
+
 [target.'cfg(target_os = "macos")']
 rustflags = [
     "-C", "link-arg=-undefined",

prql-elixir/native/prql/Cargo.toml

@@ -16,6 +16,8 @@ name = "prql"
 path = "src/lib.rs"
 
 [dependencies]
-prql-compiler = {path = "../../../prql-compiler", default-features = false, version = "0.5.2" }
-[target.'cfg(not(any(target_family="wasm")))'.dependencies]
+prql-compiler = {path = "../../../prql-compiler", default-features = false, version = "0.5.2"}
+
+# See Readme for details on Mac
+[target.'cfg(not(any(target_family="wasm", target_os = "macos")))'.dependencies]
 rustler = "0.27.0"

prql-elixir/native/prql/src/lib.rs

@@ -1,3 +1,5 @@
+// See Readme for more information on Mac compiling
+#![cfg(not(target_os = "macos"))]
 // These bindings aren't relevant on wasm
 #![cfg(not(target_family = "wasm"))]
 // TODO: unclear why we need this `allow`; it's required in `CompileOptions`,