scons icon indicating copy to clipboard operation
scons copied to clipboard

`env.File().get_env()` returns DefaultEnvironment

Open ivankravets opened this issue 3 years ago • 16 comments

Is this a feature or a bug? Related issue https://github.com/platformio/platformio-core/pull/4380

I see that SCons sets the final environment at https://github.com/SCons/scons/blob/master/SCons/Builder.py#L620 I agree with this behaviour if we use from SCons import File -> File(...).get_env() == DefaultEnvironment but not when customEnv.File().get_env() != customEnv.

Example


myenv = env.Clone()
node = myenv.File("/tmp/main.cpp")
print("%s=%s -> %s" % (id(env), id(myenv), id(node.get_env())))
> 4437414048=4451972960 -> 4437414048

ivankravets avatar Aug 05 '22 11:08 ivankravets

Hmmm, that's not what I'd "logically" expect (but then that happens to me often with SCons!). The value you get back from get_env is indeed the default environment - you can print this also to see.

print(f"{id(DefaultEnvironment())=}")

which is the expected behavior if nothing has ever "set" the node's env - and as you point out, only a builder execute method ever does that. (the Clone call in your example has nothing to do with this, FWIW). There seem to be a lot of cases where SCons tries to defer things until the last moment for doubtful performance gains, I don't know if this is a case of that. It does feel like if you do env.File() you have an association right there you could record, but perhaps that's naïve.

get_env seems to have had no clients at all in the codebase until the recently-added ninja support, and no tests, so it's hard to tell what the expectations are here. Version control history is also no help.

mwichmann avatar Aug 05 '22 13:08 mwichmann

I'd expect that env would only be set by a builder and not by File() or Dir(). So if the file in question is the target of a builder, you'd get the env that's attached to. Otherwise getting DefaultEnvironment() is not shocking.

@ivankravets - is the file in your real code a target for a builder?

bdbaddog avatar Aug 05 '22 21:08 bdbaddog

Not arguing here, just trying to understand: env.File(), env,Dir(), env.Entry() would seem to imply a clear affinity to an environment, as opposed to File(), Dir(), Entry() (unless you say those are have affinity to DefaultEnvironment)...

mwichmann avatar Aug 05 '22 22:08 mwichmann

Interesting. So it'd be subst()'d in the context of the specified env's envvars, but not attached.. hmm only env_set() sets the nodes .env property.

@ivankravets - what's your use model for calling get_env()?

bdbaddog avatar Aug 05 '22 22:08 bdbaddog

right, and as mentioned, absolutely nothing uses get_env at the moment except the quite new ninja stuff. get_env itself is not at all new, though.

mwichmann avatar Aug 05 '22 22:08 mwichmann

but code does use the node.env property without calling get_env.. (See SCons/Builder.py)

bdbaddog avatar Aug 05 '22 22:08 bdbaddog

ah, goody. that's one of the reasons I hate all of those psuedo-getter functions that Python doesn't need, and I forgot to even look for that.

mwichmann avatar Aug 05 '22 22:08 mwichmann

@bdbaddog looks like only in _node_errors?

mwichmann avatar Aug 06 '22 19:08 mwichmann

Looks that way. interesting.

bdbaddog avatar Aug 06 '22 21:08 bdbaddog

is the file in your real code a target for a builder?

@bdbaddog, yes. See line 283 in https://github.com/platformio/platformio-core/blob/develop/platformio/builder/tools/piobuild.py#L264:L304

Users in PlatformIO do not work directly with SCons if they don't need extra control. We collect source files based a on project build_src_filter. But! There are cases when an embedded developer asks PlatformIO to use external frameworks/SDKs/RTOSes. In this case, PlatformIO adds to the build stage a separate pre-configured component. Not is the question - what to do the if user wants to patch some specific external files or do other "event-based" customization?

For this use case, we added support for Build Middlewares which allow users to attach multiple handlers based on the matched patterns.

The latest documentaion is updated and includes callback(env, node) (we merged https://github.com/platformio/platformio-core/pull/4380). So, developers can now have a reference to the node's original build environment. If node.get_env() could work, we would not need this external env argument.

You can close this issue if you see it conflicts with the SCons architecture.

ivankravets avatar Aug 08 '22 14:08 ivankravets

is the file in your real code a target for a builder?

@bdbaddog, yes. See line 283 in https://github.com/platformio/platformio-core/blob/develop/platformio/builder/tools/piobuild.py#L264:L304

Users in PlatformIO do not work directly with SCons if they don't need extra control. We collect source files based a on project build_src_filter. But! There are cases when an embedded developer asks PlatformIO to use external frameworks/SDKs/RTOSes. In this case, PlatformIO adds to the build stage a separate pre-configured component. Not is the question - what to do the if user wants to patch some specific external files or do other "event-based" customization?

For this use case, we added support for Build Middlewares which allow users to attach multiple handlers based on the matched patterns.

The latest documentaion is updated and includes callback(env, node) (we merged platformio/platformio-core#4380). So, developers can now have a reference to the node's original build environment. If node.get_env() could work, we would not need this external env argument.

You can close this issue if you see it conflicts with the SCons architecture.

So you're using this to provide users a hook to call SCons builders explicitly?

bdbaddog avatar Aug 08 '22 17:08 bdbaddog

So you're using this to provide users a hook to call SCons builders explicitly?

No, they can just tune env.Object() or skip dynamically any file from the build process. Nothing more. If a user does not have any middleware, we just pass to the SCons "list of file nodes" with preconfigured env.

ivankravets avatar Aug 08 '22 17:08 ivankravets

I don't see why a node should not have a link back to its env if we know how to set it. A single assignment should not be that costly.

mwichmann avatar Aug 08 '22 20:08 mwichmann

I don't see why a node should not have a link back to its env if we know how to set it. A single assignment should not be that costly.

what if the node is source node for N variant dirs each with their own env?

bdbaddog avatar Aug 08 '22 20:08 bdbaddog

Would you have created such a node by calling env.File()? Seems unlikely, but will bow to your experience

mwichmann avatar Aug 08 '22 21:08 mwichmann

Would you have created such a node by calling env.File()? Seems unlikely, but will bow to your experience

Nope. But that's not the issue. New users seem to think they need to create a File() for everything and then use that.Which we know is not necessary. But if they do that (which they will).. it could lead to this an issue where they retrieve the DefaultEnvironment() and get a two environments same target issue.. which is maddening to resolve as it is..

bdbaddog avatar Aug 08 '22 21:08 bdbaddog