Read the Variables() saved-variables file from the source directory (#816)

[CornElDos]

Summary

Variables() reads its saved-variables file(s) (e.g. custom.py) with a hand-rolled os.path.exists() / open(), which only ever looks in the build directory. When the source directory differs from the build directory — for example when building against a separate source tree via Repository() — a custom.py placed in the source tree is not found and its values are silently ignored. This is issue #816 (the code even carried a # TODO: issue #816 marker on the offending lines).

Change

Resolve the saved-variables file through the File() node infrastructure instead of touching the filesystem directly:

• env.File(filename) with rexists() / srcnode() locates the file in the build directory, the source (variant) directory, or a repository.
• rfile().get_text_contents() reads the actual on-disk file, and rfile().get_abspath() is used for the sys.path entry so imports inside the file resolve against its real location.

This follows the approach suggested by the maintainers in the issue (use the File() infrastructure / get_text_contents()).

Testing

• New regression test test/Variables/source-dir.py builds against a separate source tree via Repository() and asserts the source-tree custom.py is applied. It fails without this change and passes with it.
• The full test/Variables/ suite passes (10 tests, no regressions).

Note: the Save() path still writes relative to the build directory (also mentioned in the original report). That is a separate concern and is intentionally left out here to keep the change focused; happy to follow up separately.

[mwichmann]

Thanks for the submission! I've been looking at related issues, where the variantdir mechanism (which counts on remapping filenames if in effect), and to some extent Repository as well, are bypassed by code going directly to the file system. I will say it's little murky what the intent is when the object in question isn't going to be part of the build graph - the saved-variables file being one of those cases. Should those be relocated or not? I guess you're prodding us to answer that question...

[CornElDos]

Thanks for taking a look, Mats! You've put your finger on exactly the thing here. My reasoning for relocating the saved-variables file: in practice custom.py is usually checked into the source tree next to the SConstruct, and building in a variant dir or against a Repository is common. So when SCons silently ignores the custom.py sitting right there in the source tree (because the raw os.path.exists() only looks in the build dir), it reads as a bug, which is what the original 2004 report and #816 describe. Routing it through File() makes it behave the way a user would expect for a file they authored. That said, I take your point that it's murky in general for objects outside the build graph, and I don't want to pre-empt a broader decision. Happy to narrow the scope however you prefer, only handle the Repository case, gate it behind a flag, or hold off until the general policy is settled. I've also just pushed a fix for the unit-test regression (the test stub Environment didn't implement File()), so CI should be green now. Glad it prompted the question. Happy to iterate.

…

On Wed, 8 Jul 2026 at 15:13, Mats Wichmann ***@***.***> wrote: *mwichmann* left a comment (SCons/scons#4875) <#4875 (comment)> Thanks for the submission! I've been looking at related issues, where the variantdir mechanism (which counts on remapping filenames if in effect), and to some extent Repository as well, are bypassed by code going directly to the file system. I will say it's little murky what the *intent* is when the object in question isn't going to be part of the build graph - the saved-variables file being one of those cases. Should those be relocated or not? I guess you're prodding us to answer that question... — Reply to this email directly, view it on GitHub <#4875?email_source=notifications&email_token=A7RVDS2CMQM5HASNDNIEFRL5DZCGVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRGUYTINRZGI42M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4915146929>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A7RVDS7AXDYA5VIDIAXFAO35DZCGVAVCNFSNUABFKJSXA33TNF2G64TZHMYTANBWG4YDCNRQHNEXG43VMU5TIOBTGY3TIMZWHAZ2C5QC> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/A7RVDS66OE22QVKQWJ6YRWL5DZCGVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRGUYTINRZGI42M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/A7RVDS3KYLTY5RWCS7MUTWD5DZCGVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRGUYTINRZGI42M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you authored the thread.Message ID: ***@***.***>

[mwichmann]

I've done some more digging since I'm interested in the general topic, and there's a fair bit to consider. Even though I added the TODO lines which asked if we should use the node system to resolve the path (fairly recently - only about two years ago), I'm not currently convinced that adding a saved-variables file to the node graph is the best approach. There are other ways to resolve the path that should work (SConstruct-relative). Meanwhile, there are expectations built in to existing tests about paths - see test/Variables/chdir.py and test/Variables/import.py - they use a trick with a relative path which seems dodgy to me (as in, may have come from: "oops, this test didn't work. How can I fiddle it so it passes"), but could have been intentional - see the line opts = Variables('../bin/opts.cfg', ARGUMENTS).

[CornElDos]

Thanks for digging in, Mats. I appreciate you taking it seriously :) (There's also something quietly relatable about a two-year-old TODO you left for yourself now getting cross-examined by present-you. Been there.) You're right to be wary of pulling a non-build file into the node graph. That's the real cost of my approach, and it's a fair objection. Here's how I see the paths, because I think the deciding question is narrower than it looks: does the fix need to handle Repository(), or just variant dirs? That's the fork: 1. SConstruct-relative resolution (your instinct). Cleanest, no node graph involved, and it handles the variant-dir case directly. The catch is the original 2004 report: a custom.py living in a separate source tree built against via Repository(). To cover that, SConstruct-relative resolution has to additionally walk the repository search path. Otherwise it fixes variant dirs but not the Repository scenario #816 actually describes. Doable, just a bit more than a plain relative join. 2. Node system, strictly for path resolution. Use File() only to resolve the location (it already knows variant dirs and repositories), read with a plain open(), and never treat it as a buildable node. That's close to what the PR does. If the objection is "it's in the graph," I'm glad to make the "resolve, don't build" boundary explicit. 3. Scope to the reported case only. If Repository support is out of scope for now, a targeted SConstruct-relative fix is the smallest change, and we leave the repository question to a follow-up. On chdir.py and import.py: agreed, the Variables('../bin/opts.cfg', ARGUMENTS) relative trick looks like it grew from "make the test pass" more than from intent. But until we know, whatever lands shouldn't quietly change what those rely on. My current change keeps them green, which isn't the same as knowing they're right. Happy to take it whichever way you prefer. I can rework toward (1) with repository support for the general fix, or trim to (3) to keep it minimal. Your call on direction, and I'll do the work.

…

On Wed, 8 Jul 2026 at 17:58, Mats Wichmann ***@***.***> wrote: *mwichmann* left a comment (SCons/scons#4875) <#4875 (comment)> I've done some more digging since I'm interested in the general topic, and there's a fair bit to consider. Even though I added the TODO lines which asked if we should use the node system to resolve the path (fairly recently - only about two years ago), I'm not currently convinced that adding a saved-variables file to the node graph is the best approach. There are other ways to resolve the path that should work (SConstruct-relative). Meanwhile, there are expectations built in to existing tests about paths - see test/Variables/chdir.py and test/Variables/import.py - they use a trick with a relative path which seems dodgy to me (as in, may have come from: "oops, this test didn't work. How can I fiddle it so it passes"), but *could* have been intentional - see the line opts = Variables('../bin/opts.cfg', ARGUMENTS). — Reply to this email directly, view it on GitHub <#4875?email_source=notifications&email_token=A7RVDS4GCFKS7URI5GOHPV35DZVR5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRGY3TAMRUG4YKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4916702470>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A7RVDSYBSZ5KMGUMQVHU6AL5DZVR5AVCNFSNUABFKJSXA33TNF2G64TZHMYTANBWG4YDCNRQHNEXG43VMU5TIOBTGY3TIMZWHAZ2C5QC> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/A7RVDSYQ4N5CKUKYEAQZFQ35DZVR5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRGY3TAMRUG4YKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/A7RVDSZOCBPDGVULWLOCZXL5DZVR5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRGY3TAMRUG4YKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you authored the thread.Message ID: ***@***.***>

[mwichmann]

There's another point, perhaps not fair to layer it onto a quite specific PR... in all existing versions, you can't use top-relative addressing for specifying saved-variables files (#/saved.py). Using File() magically "fixes" that, although it was never documented as possible; other approaches would have to do a bit more work.

[CornElDos]

Good point, and I don't think it's unfair to raise. Honestly it cuts in favor of the node approach for me: File() isn't fixing one case, it's the one resolver that already understands variant dirs, repositories, and #-top-relative addressing all at once. Any SConstruct-relative version would have to re-derive each of those by hand, and probably land back at reimplementing a chunk of the node path logic. The #/saved.py support being undocumented-but-now-working is a fair flag. Easy enough to either document it as intended, or explicitly reject #-paths if you'd rather not widen the surface here. If the only remaining hesitation is "it's in the build graph," the middle option still stands: use File() purely to resolve the path, read with open(), and never register it as buildable. Same reach, no graph membership. Whatever you land on for scope, I'm happy to shape the PR to match.

…

On Wed, 8 Jul 2026 at 19:34, Mats Wichmann ***@***.***> wrote: *mwichmann* left a comment (SCons/scons#4875) <#4875 (comment)> There's another point, perhaps not fair to layer it onto a quite specific PR... in all existing versions, you can't use top-relative addressing for specifying saved-variables files (#/saved.py). Using File() magically "fixes" that, although it was never documented as possible; other approaches would have to do a bit more work. — Reply to this email directly, view it on GitHub <#4875?email_source=notifications&email_token=A7RVDS3CPEG6PBMBXAAXP535D2AY7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRG42TAMBVGY22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4917500565>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A7RVDS7DEGPGVPRTGOOYL3L5D2AY7AVCNFSNUABFKJSXA33TNF2G64TZHMYTANBWG4YDCNRQHNEXG43VMU5TIOBTGY3TIMZWHAZ2C5QC> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/A7RVDSYTKBQVZECAJU43MET5D2AY7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRG42TAMBVGY22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG> and Android <https://github.com/notifications/mobile/android/A7RVDS26X4GO6IUEWWQRQRT5D2AY7A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRG42TAMBVGY22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>. Download it today! You are receiving this because you authored the thread.Message ID: ***@***.***>

[bdbaddog]

At first glance, seems like all this can be replaced by
contents=File(filename).get_text_contents()

Any reason (that I"m missing) not to do that?

[bdbaddog]

Can you just import File and use that directly?

[bdbaddog]

Looks good.

I added a few comments which may simplify your logic.

Also, please add a blurb to RELEASE.txt