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

sys: copy files only if needed #9

Merged
merged 4 commits into from
Sep 7, 2023
Merged

sys: copy files only if needed #9

merged 4 commits into from
Sep 7, 2023

Conversation

0x2ec
Copy link
Contributor

@0x2ec 0x2ec commented Sep 6, 2023

The cmake build takes care of creating a build dir somewhere in OUT_DIR, so there shouldn't be any need to copy all the sources by hand. This also fixes a problem where Cargo always rebuilds the -sys crate, because the file copies essentially amount to a "touch" on all the files copied to OUT_DIR.

@0x2ec
Copy link
Contributor Author

0x2ec commented Sep 6, 2023

I added some small stylistic changes, only because I like fiddling it 😄 feel free to drop those patches if you don't want them.

@Joylei
Copy link
Owner

Joylei commented Sep 6, 2023

The reason to copy source is that the libplctag was trying to modify source to replace version string while building it, which is not allowed by rust build.
Let me check how it works now.

@0x2ec
Copy link
Contributor Author

0x2ec commented Sep 6, 2023

The reason to copy source is that the libplctag was trying to modify source to replace version string while building it, which is not allowed by rust build. Let me check how it works now.

That's... interesting behaviour. I thought cmake would copy sources into its build dir, and any modification would happen there. In any case, with 0d2b4df applied, running cargo b --all --examples I don't see any modified files in the repo.

But I can try to dig a bit more into libplctag's build system to see if I can make sense of it.

@0x2ec
Copy link
Contributor Author

0x2ec commented Sep 6, 2023

@Joylei I guess you'd be referring to https://github.com/libplctag/libplctag/blob/release/CMakeLists.txt#L327, right?

CONFIGURE_FILE("${lib_SRC_PATH}/version.h.in" "${lib_SRC_PATH}/version.h" @ONLY)

yeah, removing the file makes it reappear when doing a clean build, I guess that's not good.

@0x2ec
Copy link
Contributor Author

0x2ec commented Sep 6, 2023

An alternative could be to pull in something like dircpy and set it up like this:

let source_dir = "libplctag";
// fix publish issue: Build scripts should not modify anything outside of OUT_DIR
let dst_dir = PathBuf::from(env::var("OUT_DIR").unwrap()).join("libplctag");
CopyBuilder::new(source_dir, dst_dir)
    .overwrite_if_newer(true)
    .run()
    .unwrap();

would you prefer that?

The overwrite_if_newer() checks the mtime on the source and destination, and copies only if they differ.

@0x2ec
Copy link
Contributor Author

0x2ec commented Sep 6, 2023

I could also just expand the existing functionality to check mtime prior to a copy, and only copy when mtimes differ...

Copy files only if they don't already exist or their mtimes are
different.
Some imports were only used on windows targets.
@0x2ec 0x2ec changed the title sys: avoid manual copy of sources sys: copy files only if needed Sep 7, 2023
@Joylei Joylei merged commit 0bb75fb into Joylei:master Sep 7, 2023
0 of 3 checks passed
@0x2ec
Copy link
Contributor Author

0x2ec commented Sep 7, 2023

jus saw the merge, thanks \o/

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