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

The embed_migrations! macro doesn't pick up new migration files due to cargo build cache #3119

Open
3 tasks done
fbecart opened this issue Apr 5, 2022 · 5 comments
Open
3 tasks done
Labels

Comments

@fbecart
Copy link

fbecart commented Apr 5, 2022

Setup

Versions

  • diesel: 1.4.8
  • diesel_migrations: 1.4.0
  • migrations_macros: 1.4.2

Problem Description

I use the embed_migrations! to generate a binary to run Diesel migrations. Cargo will sometimes reuse the previously built binary instead of building a new one, because it didn't realize its cache became invalid. This happens when I'm only adding new files to the migrations directory.

What are you trying to accomplish?

Build a binary to run the Diesel migrations.

What is the expected output?

The binary produced by cargo build should run all of the migration files, including the most recent ones.

What is the actual output?

Cargo didn't rebuild the crate, and the binary is now outdated. As a result, if I run the binary, recent migrations will be missing.

Are you seeing any additional errors?

No

Steps to reproduce

/src/bin/run-migrations.rs:

#[macro_use]
extern crate diesel_migrations;

use common_infra::config;
use diesel::{pg::PgConnection, prelude::*};

// path relative to Cargo.toml
embed_migrations!("./migrations");

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let connspec = config::get_plain_url("Database")?;
    let conn = PgConnection::establish(&connspec)?;

    info!("Applying migrations");
    embedded_migrations::run_with_output(&conn, &mut std::io::stdout())?;
    Ok(())
}
  • Run cargo build
  • Add a new migration
  • Run cargo build again, and observe that cargo skipped the build.

The resulting binary misses the additional SQL migration.

Checklist

  • I have already looked over the issue tracker and the discussion forum for similar possible closed issues.
  • This issue can be reproduced on Rust's stable channel. (Your issue will be
    closed if this is not the case)
  • This issue can be reproduced without requiring a third party crate

Workaround

It is possible to invalidate the build cache with a /build.rs file:

use std::fs::create_dir_all;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    println!("cargo:rerun-if-changed=migrations");
    Ok(())
}
@fbecart fbecart added the bug label Apr 5, 2022
@weiznich
Copy link
Member

weiznich commented Apr 5, 2022

Thanks for opening this bug report. Can you check if the same thing happens with the implementation on the master branch? The embed_migrations!() feature got a complete rewrite there and now uses compile builtins to include the migration sql. So hopefully that fixed the issue.
Otherwise there is really not much we can do here as the build.rs solution needs to be done by the corresponding downstream crate and there is no corresponding API for proc macros to signal that a macro needs to be rerun if some input changed.
If it's not fixed we should add a note in our documentation about this issue.

@fbecart
Copy link
Author

fbecart commented Apr 6, 2022

Thanks @weiznich for the swift reply.

I was able to reproduce the issue on the master branch. I had to update my main.rs slightly:

use diesel::{pg::PgConnection, prelude::*};
use diesel_migrations::EmbeddedMigrations;
use diesel_migrations::{embed_migrations, MigrationHarness};

pub const MIGRATIONS: EmbeddedMigrations = embed_migrations!("./migrations");

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let mut conn = PgConnection::establish("postgres://postgres@localhost:1234/test")?;

    println!("Migrating...");
    conn.run_pending_migrations(MIGRATIONS).unwrap();
    Ok(())
}

The issue is the same as documented before: cargo does not see the new SQL migration scripts, and reuses the outdated binary. As a result, the latest migration scripts are missing from the binary.

@fbecart
Copy link
Author

fbecart commented Apr 6, 2022

The issue shows up here: https://github.com/diesel-rs/diesel/blob/master/diesel_migrations/migrations_internals/src/lib.rs#L89

cargo doesn't have the same level of integration with read_dir as it has with include_str! for instance.

@weiznich
Copy link
Member

weiznich commented Apr 6, 2022

Fixing this requires the stabilization of proc_macro::tracked_path. So this is blocked on rust-lang/rust#73921 for now.
I would likely accept a PR that uses this unstable API for nightly builds only.

@weiznich
Copy link
Member

weiznich commented Apr 6, 2022

launchbadge/sqlx#1332 might be relevant to see how others fixing a similar issue.

weiznich added a commit to weiznich/diesel that referenced this issue May 31, 2022
weiznich added a commit to weiznich/diesel that referenced this issue Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants