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

Fix issue causing scanners to receive incorrect paths #3494

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

grossag
Copy link
Contributor

@grossag grossag commented Dec 13, 2019

This pull request resolves an issue where scanners receive an incorrect path if they use PathList as their path_function. It also includes a test to demonstrate the issue.

PathList can be used by scanners to easily get a list of paths corresponding to a specific variable in the environment. For example, the ProgramScanner sets its path_function to the result of the call SCons.Scanner.FindPathDirs('LIBPATH').

The issue happens when the following conditions are all true (demonstrated by the included test):

  • The environment contains a Directory node that is a subdirectory of the source root directory.
  • The environment variable used by the scanner (e.g. env['LIBPATH']) references a subdirectory of that directory node.
  • A SConscript file is being run that is not in the source root directory.

Here is a summary of why the issue is happening. The PathList class converts the directory to a string, thus turning it into a relative path to the source root. When the PathList class evaluates the string (e.g. "$SDKROOT/include") it substitutes the directory to a string and ends up with a path that is correct relative to the source root. However, it then applies that path as being relative to the SConscript file, which is not correct.

I am not wedded to the current fix but it is the most compelling fix that I have been able to find. In this case it is dangerous to operate on relative paths, so the fix is to operate on absolute paths when doing string substitution in the PathList class. Object substitution (e.g. if the string being substituted is "$SDKROOT" instead of "$SDKROOT/include") continues to work as before.

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated master/src/CHANGES.txt directory (and read the README.txt in that directory)
  • I have updated the appropriate documentation

@grossag grossag changed the title [WIP] Test demonstrating incorrect path received by a scanner Fix issue causing scanners to receive incorrect paths Jan 10, 2020
This change does the following:

1. Modifies CHANGES.txt to include a description of the fix.
2. Applies the preferred fix that I have been testing for a few weeks.
@grossag
Copy link
Contributor Author

grossag commented Jan 15, 2020

Thoughts on this patch? I'm open to alternative solutions if it has risks that I am unaware of.

@bdbaddog
Copy link
Contributor

O.k. did some digging.

So the issue is if the value returned from _PathList.subst_path() is a string and not a node, then Base.RFindalldirs() will append it to the current dir (via Dir.get_all_rdirs())

If you change your test to the following:

env['SDKROOT'] = env.Dir('sdk')
env.Replace(
    PYTHON=sys.executable,
    PYPATH=[env.Dir('$SDKROOT'), env.Dir('$SDKROOT/sdk_subdir')],

Then your test passes without any code modifications.
Note that you need to set SDKROOT before env.Replace's env.Dir with $SDKROOT in it.

This is in line with the expected expansion of any env variable which refers to a File Node from any other directory but the base directory if I'm remembering correctly.

Is that a viable change for you?

Seems like the best fix would be to pass the cwd into the subst_path() and then have subst itself do the right thing with any nodes it evaluates..

I think your fix, while working for you is not a general purpose fix because it overrides the existing node_conv() function which will call str(arg) in some cases which is not happening with your lambda.

Lemme ponder a bit more. but let us know if you can use actual nodes in your path list?

@bdbaddog
Copy link
Contributor

Also if you allow me write access to this PR branch I'll push fixes to the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants