SCons / scons

SCons - a software construction tool
http://scons.org
MIT License
2.11k stars 319 forks source link

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

Open ivankravets opened 2 years ago

ivankravets commented 2 years ago

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
mwichmann commented 2 years ago

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.

bdbaddog commented 2 years ago

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?

mwichmann commented 2 years ago

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)...

bdbaddog commented 2 years ago

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()?

mwichmann commented 2 years ago

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.

bdbaddog commented 2 years ago

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

mwichmann commented 2 years ago

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 commented 2 years ago

@bdbaddog looks like only in _node_errors?

bdbaddog commented 2 years ago

Looks that way. interesting.

ivankravets commented 2 years ago

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.

bdbaddog commented 2 years ago

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?

ivankravets commented 2 years ago

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.

mwichmann commented 2 years ago

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.

bdbaddog commented 2 years ago

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?

mwichmann commented 2 years ago

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

bdbaddog commented 2 years ago

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..