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

fix(prepare): store temporary query files inside the workspace #2398

Merged
merged 1 commit into from
Mar 15, 2023

Conversation

aschey
Copy link
Contributor

@aschey aschey commented Mar 11, 2023

Resolves #2395

Thanks to @cycraig for the advice here: #2395 (comment)

The PR to implement one file per query: #2363 implemented logic to write queries to a temp file and then persist them to SQLX_OFFLINE_DIR here:

let mut tmp_file = tempfile::NamedTempFile::new()
.map_err(|err| format!("failed to create query file: {:?}", err))?;
serde_json::to_writer_pretty(tmp_file.as_file_mut(), self)
.map_err(|err| format!("failed to serialize query data to file: {:?}", err))?;
tmp_file
.persist(dir.as_ref().join(format!("query-{}.json", self.hash)))
.map_err(|err| format!("failed to move query file: {:?}", err))?;

However, the docs for NamedTempFile::persist mention the following:

Note: Temporary files cannot be persisted across filesystems.

On Linux, the /tmp directory is commonly mounted as tmpfs which means the call to tmp_file.persist will result in the following error: failed to move query file: PersistError(Os { code: 18, kind: CrossesDevices, message: "Invalid cross-device link" }).

To prevent this, we can't rely on using the default temp directory. This PR changes the logic to use NamedTempFile::new_in to create the temporary files in a new variable called SQLX_TMP. When run from cargo sqlx prepare, this is set to $TARGET_DIR/sqlx-tmp.

The point of the SQLX_TMP variable is to ensure the temporary files are written to a directory that isn't picked up by git. As long as everything completes successfully, this shouldn't really matter as the temp files will get cleaned up automatically on a clean exit. Ideally, we wouldn't need this and could instead just reference the target directory directly, but that would require a call to cargo metadata from sqlx-macros-core.

I'm not too familiar with this logic so please let me know if there's a better way to handle this. Just thought I'd take a stab at it, thanks!

@abonander
Copy link
Collaborator

abonander commented Mar 14, 2023

I would definitely prefer if it defaulted to target/, I don't want to require users to have to set another temporary variable.

I believe the working directory for proc macro execution is the workspace root, so we could just default to target/sqlx/. I don't see any reason not to provide an environment variable override, I just don't want it to be required. If CARGO_TARGET_DIR is set we should also respect that.

@abonander
Copy link
Collaborator

abonander commented Mar 14, 2023

The other option is to use the .sqlx directory or a subdirectory but default-generate a .gitignore file that excludes the temporary directory or files (using, e.g. .json.tmp as an extension). That might play better with non-Cargo build setups or weird configurations.

@aschey
Copy link
Contributor Author

aschey commented Mar 14, 2023

Okay, made the change to fall back to CARGO_TARGET_DIR and then to ./target/sqlx. As long as the process doesn't panic in the short time between creating the temp file and the variable going out of scope, I think there's a pretty small chance these temp files don't get cleaned up, so I think this is pretty low risk. With that in mind, I went with the simpler solution, but please let me know if you'd like me to change anything else.

@abonander abonander merged commit cf3ce13 into launchbadge:main Mar 15, 2023
@aschey aschey deleted the prepare-tmp-dir branch March 13, 2024 02:02
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.

0.7.0-alpha.1: error: failed to move query file when running cargo sqlx prepare
2 participants