Cleanup `build.rs` and friends issue #1111 #1112

eeeebbbbrrrr · 2023-04-18T19:12:36Z

This PR "cleans up" build.rs so that it now tracks, via cargo:rerun-if-env-changed, every environment variable it uses. The justification here is that if build.rs uses an environment variable, then build.rs will need to re-run if that envar changes. It could be this is overly aggressive in that cargo probably does this for us for things like TARGET or CARGO_MANIFEST_DIR or OUT_DIR, but it seems prudent to not have to then justify why we track some envars and not others.

It also doesn't try to open $PGRX_HOME/config.toml in the case where the pg_config data is wholly described as environment variables.

And it also removes from Pgrx::home() the code that would just blindly try to create $PGRX_HOME if it doesn't exist. It instead moves that to the Err case when it's called via cargo pgrx init.

…X_HOME, should not also be creating that path.

workingjubilee · 2023-04-19T17:37:54Z

I'm throwing the Will It Blend tests at this just to make sure.

workingjubilee

Largely looks good. Some mild nits. Somewhat concerned about duplicating the tracking command confusing cargo.

cargo-pgrx/src/command/init.rs

workingjubilee · 2023-04-19T19:35:00Z

pgrx-pg-sys/build.rs

-        == "1";
-    println!("cargo:rerun-if-env-changed=PGRX_PG_SYS_GENERATE_BINDINGS_FOR_RELEASE");
+    let is_for_release =
+        env_tracked("PGRX_PG_SYS_GENERATE_BINDINGS_FOR_RELEASE").unwrap_or("0".to_string()) == "1";


Trivial, I know, and preexisting, but it bugs me these aren't consistent.

Suggested change

env_tracked("PGRX_PG_SYS_GENERATE_BINDINGS_FOR_RELEASE").unwrap_or("0".to_string()) == "1";

env_tracked("PGRX_PG_SYS_GENERATE_BINDINGS_FOR_RELEASE").unwrap_or_else(|| "0".to_string()) == "1";

Also looking again this should actually be another case of .as_ref() == Some("1")

pgrx-pg-sys/build.rs

workingjubilee · 2023-04-19T19:43:25Z

pgrx-pg-sys/build.rs

@@ -760,7 +763,7 @@ fn env_tracked(s: &str) -> Option<String> {
 }

 fn target_env_tracked(s: &str) -> Option<String> {
-    let target = std::env::var("TARGET").unwrap();
+    let target = env_tracked("TARGET").unwrap();


I'm surprised that we need to add env_tracked to this, I thought Cargo was already tracking this? Are we sure we won't confuse Cargo on this point?

workingjubilee · 2023-04-19T19:46:17Z

pgrx-pg-sys/build.rs

+        let manifest_dir = env_tracked("CARGO_MANIFEST_DIR").map(PathBuf::from).unwrap();
+        let out_dir = env_tracked("OUT_DIR").map(PathBuf::from).unwrap();


Same concerns re: "should we really be adding this to the env tracking"?

eeeebbbbrrrr · 2023-04-19T19:48:48Z

so... I blindly did this for every environment variable. My thought process was that if build.rs uses an envar, then it'll need to be rerun when that envar changes.

workingjubilee · 2023-04-19T19:50:16Z

Whee, errant mouseclicks.

Co-authored-by: Jubilee <[email protected]>

workingjubilee · 2023-04-19T19:56:02Z

Hmm. Given the big build test suite passed on a previous version of this PR, we should be okay? I feel like my main concern is really just making sure the commit message or a comment or whatever includes "yeah this was a more... interesting decision" so that if we find an unusual bug later related to the build, shuffling through the git blame eventually turns up something.

eeeebbbbrrrr · 2023-04-19T20:06:26Z

Hmm. Given the big build test suite passed on a previous version of this PR, we should be okay? I feel like my main concern is really just making sure the commit message or a comment or whatever includes "yeah this was a more... interesting decision" so that if we find an unusual bug later related to the build, shuffling through the git blame eventually turns up something.

I updated the PR description. If that works for you, it'll become the commit message -- once CI passes the additional cleanups I just pushed.

workingjubilee

Yep, looks good to me!

thomcc · 2023-04-19T21:36:31Z

It could be this is overly aggressive in that cargo probably does this for us for things like TARGET or CARGO_MANIFEST_DIR or OUT_DIR, but it seems prudent to not have to then justify why we track some envars and not others.

There used to be issues where doing this for the ones cargo manages would cause spurious rebuilds. I think these have been fixed, but it's possibly worth checking.

If it helps, https://github.com/rust-lang/cc-rs/blob/main/src/lib.rs#L3132-L3139 is what I've used in the past (although the main reason cc-rs tries to avoid it is not that bug, but because emitting any rerun-if-env-changed vars can impact if rebuilds happen).

eeeebbbbrrrr · 2023-04-20T14:06:46Z

If it helps, https://github.com/rust-lang/cc-rs/blob/main/src/lib.rs#L3132-L3139 is what I've used in the past (although

It's funny, I had this same thought yesterday afternoon while walking the dog.

the main reason cc-rs tries to avoid it is not that bug, but because emitting any rerun-if-env-changed vars can impact if rebuilds happen).

Well, we use rerun-if-env-changed to cause rebuilds, so like, /shrug?

I feel like this should be fine. If it causes rebuilds when it shouldn't then I'd argue that's a cargo bug and we should report it there and then work around it like you do in cc-rs. I'm gonna go ahead and merge and we can see what happens during our daily development.

eeeebbbbrrrr · 2023-04-27T14:49:45Z

I feel like this should be fine. If it causes rebuilds when it shouldn't then I'd argue that's a cargo bug and we should report it there and then work around it like you do in cc-rs. I'm gonna go ahead and merge and we can see what happens during our daily development.

As usual, @thomcc was correct! :)

I've put up PR #1127 to address the constant rebuilds that started happening after this PR was merged.

I didn't narrow it down specifically because I got bored, but it's at least caused by one of the envars we access that begins with "CARGO": CARGO_CFG_TARGET_OS, CARGO_FEATURE_PGxx, or CARGO_CFG_PLRUSTC.

I still say this is some kind of cargo bug. Crate authors shouldn't have to do this.

Pgrx::home(), the static function that figures out the path for $PG…

a413aeb

…X_HOME, should not also be creating that path.

eeeebbbbrrrr changed the title ~~WIP: Cleanup buildrs and friends issue #1111~~ WIP: Cleanup build.rs and friends issue #1111 Apr 18, 2023

eeeebbbbrrrr added 2 commits April 18, 2023 15:29

do a better job tracking environment variables that are used

d7399c9

don't try to read $PGRX_HOME/config.toml if we don't need to.

d852e5e

eeeebbbbrrrr changed the title ~~WIP: Cleanup build.rs and friends issue #1111~~ Cleanup build.rs and friends issue #1111 Apr 18, 2023

eeeebbbbrrrr marked this pull request as ready for review April 18, 2023 19:35

eeeebbbbrrrr requested a review from workingjubilee April 18, 2023 19:38

eeeebbbbrrrr mentioned this pull request Apr 19, 2023

build.rs requires $PGRX_HOME/config.toml when it doesn't need to #1111

Closed

workingjubilee reviewed Apr 19, 2023

View reviewed changes

Merge branch 'develop' into cleanup-buildrs-and-friends-issue1111

ab056eb

workingjubilee closed this Apr 19, 2023

workingjubilee reopened this Apr 19, 2023

eeeebbbbrrrr and others added 4 commits April 19, 2023 15:50

Update cargo-pgrx/src/command/init.rs

a17fc71

Co-authored-by: Jubilee <[email protected]>

Update pgrx-pg-sys/build.rs

5446295

Co-authored-by: Jubilee <[email protected]>

Update pgrx-pg-sys/build.rs

b08f04b

Co-authored-by: Jubilee <[email protected]>

code review cleanup

05b5ab1

code review cleanup

198b973

workingjubilee approved these changes Apr 19, 2023

View reviewed changes

eeeebbbbrrrr merged commit 9d0ce83 into develop Apr 20, 2023

eeeebbbbrrrr mentioned this pull request Apr 24, 2023

Update version to 0.8.0 #1121

Merged

eeeebbbbrrrr deleted the cleanup-buildrs-and-friends-issue1111 branch June 20, 2023 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup `build.rs` and friends issue #1111 #1112

Cleanup `build.rs` and friends issue #1111 #1112

eeeebbbbrrrr commented Apr 18, 2023 •

edited

Loading

workingjubilee commented Apr 19, 2023

workingjubilee left a comment

workingjubilee Apr 19, 2023

workingjubilee Apr 19, 2023

workingjubilee Apr 19, 2023

workingjubilee Apr 19, 2023

eeeebbbbrrrr commented Apr 19, 2023

workingjubilee commented Apr 19, 2023

workingjubilee commented Apr 19, 2023

eeeebbbbrrrr commented Apr 19, 2023

workingjubilee left a comment

thomcc commented Apr 19, 2023

eeeebbbbrrrr commented Apr 20, 2023

eeeebbbbrrrr commented Apr 27, 2023

	env_tracked("PGRX_PG_SYS_GENERATE_BINDINGS_FOR_RELEASE").unwrap_or("0".to_string()) == "1";
	env_tracked("PGRX_PG_SYS_GENERATE_BINDINGS_FOR_RELEASE").unwrap_or_else(\|\| "0".to_string()) == "1";

		let manifest_dir = env_tracked("CARGO_MANIFEST_DIR").map(PathBuf::from).unwrap();
		let out_dir = env_tracked("OUT_DIR").map(PathBuf::from).unwrap();

Cleanup build.rs and friends issue #1111 #1112

Cleanup build.rs and friends issue #1111 #1112

Conversation

eeeebbbbrrrr commented Apr 18, 2023 • edited Loading

workingjubilee commented Apr 19, 2023

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee Apr 19, 2023

Choose a reason for hiding this comment

workingjubilee Apr 19, 2023

Choose a reason for hiding this comment

workingjubilee Apr 19, 2023

Choose a reason for hiding this comment

workingjubilee Apr 19, 2023

Choose a reason for hiding this comment

eeeebbbbrrrr commented Apr 19, 2023

workingjubilee commented Apr 19, 2023

workingjubilee commented Apr 19, 2023

eeeebbbbrrrr commented Apr 19, 2023

workingjubilee left a comment

Choose a reason for hiding this comment

thomcc commented Apr 19, 2023

eeeebbbbrrrr commented Apr 20, 2023

eeeebbbbrrrr commented Apr 27, 2023

Cleanup `build.rs` and friends issue #1111 #1112

Cleanup `build.rs` and friends issue #1111 #1112

eeeebbbbrrrr commented Apr 18, 2023 •

edited

Loading