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

Test for how di files are handled with variant dir. #4372

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

Conversation

Alexibu
Copy link
Contributor

@Alexibu Alexibu commented Jul 6, 2023

Hi,
The new di file work recently merged doesn't work with variant dirs. The original code I started this branch with worked, although I did have to include the di files with ../include.

In this pull request there is an additional test which uses a variant dir which fails.

I am hoping maintainers can have a look at it, and point me in the right direction how to fix it.
Alex

@Alexibu
Copy link
Contributor Author

Alexibu commented Jul 6, 2023

The new test passes, but only by setting env['VARIANT_DIR'] to the same as the variant_dir argument to SConscript.

@mwichmann
Copy link
Collaborator

mwichmann commented Jul 6, 2023

To save a bit of time, can you describe "doesn't work" in words? Currently wrestling with something in FortranCommon that could be of similar origin.

@Alexibu
Copy link
Contributor Author

Alexibu commented Jul 6, 2023

It doesn't prepend the path that was passed in the variant_dir argument to the DI_FILE_DIR expansion, so the di targets end up in the wrong place and the build fails.

@Alexibu Alexibu changed the title Test for how di handles are handled with variant dir. Test for how di files are handled with variant dir. Jul 6, 2023
@mwichmann
Copy link
Collaborator

That does sound like one of the problems I've been chasing - and not completely solving yet. SCons makes the correct handling of these "extras" (that's not the right word I know) a lot harder than you think it should be.

I'll try to take a look later. Meanwhile, usually the dynamic construction of a flag uses _concat which goes through a path that at least has a chance of accounting for the variantdir. It also evaluates to an empty string if the second argument is not set, so you don't have to add a separate check for that. Can you see if doing this helps:

    env['_DI_FLAGS'] = "$( ${_concat(DI_FILE_DIR_PREFIX, DI_FILE_DIR, DI_FILE_DIR_SUFFFIX, __env__, lambda x: x, TARGET, SOURCE)} $)

The usual stanza here seems to be to use RDirs as the function rather than the no-action lambda, but that's not right for a directory (if you use a variant dir with duplicate=False, RDir will generate two directories in that case. In Fortran, multiple -J moduledir flags fails the compiler. The current master still has it using RDirs).

@mwichmann
Copy link
Collaborator

Mmm, perhaps not the lamba, it doesn't expand #. So perhaps:

    env['_DI_FLAGS'] = "$( ${_concat(DI_FILE_DIR_PREFIX, DI_FILE_DIR, DI_FILE_DIR_SUFFFIX, __env__, Dirs, TARGET, SOURCE)} $)

@Alexibu
Copy link
Contributor Author

Alexibu commented Jul 6, 2023

Using _concat with Dirs does prepends the variant directory, but it also prepends the path elements of the source file between file name and source dir. In the test case this results in build/parts/include instead of build/include

@Alexibu
Copy link
Contributor Author

Alexibu commented Jul 6, 2023

It would be reasonable to copy the source directory structure and have debug_build/include/parts/part.di so that the part.di can be included with import parts.part, but you can't just transfer it across without the include or the compiler will find the source .d file either in the source directory or if files are duplicated in the build directory.

With SCons you can build a target using Object(target, source), and if you don't provide a target it copies the structure across as a default behaviour. To replicate this behaviour for DI files, we would need to be able to individually specify DI_FILE_DIR for each object in a script to control where they go, and also have the default behaviour to replicate the directory structure under a root include directory. Because of the way SCons works with variables, it would have no way of telling which one you meant, so we would need to introduce a second variable like DI_FILE_DIR for direct control and DI_ROOT_DIR for structure copying behaviour.

Either way, I can't figure out how to do either at the moment.

@mwichmann
Copy link
Collaborator

mwichmann commented Jul 6, 2023

also prepends the path elements of the source file

And that's not correct, I presume. The model is that a reference to a file or directory name is interpreted as a relative path. A lot of the doc wording says "relative to the directory of the sconscript" which is imprecise (in a non-duplicating variantdir those aren't the same thing). It will work differently if your _concat call omits the final two args (SOURCE, TARGET), but I'm not sure if that will be better... still trying to wrap my head around this.

@Alexibu
Copy link
Contributor Author

Alexibu commented Jul 6, 2023

If you did have source files in multiple locations the build wouldn't make sense.

x.d
parts/y.d
parts/subpart/z.d

would generate di files like :

include/x.di
parts/include/y.di
parts/subpart/include/z.di

Which would not be useful

@Alexibu
Copy link
Contributor Author

Alexibu commented Jul 6, 2023

You would either want to be able to copy them all to a single directory (which would also allow individual control of where di went), so you can include them with -Iinclude and import x; import y; import z;

include/x.di
include/y.di
include/z.di

or copy the structure under the include path so you can include them with -Iinclude and import x; import parts.y;

import parts.subpart.z;
include/x.di
include/parts/y.di
include/parts/subpart/z.di

@mwichmann
Copy link
Collaborator

Trying to get back to this with one of a couple of patchsets I have... these tests don't seem to fail on unmodified SCons head, which I expected. What am I missing?

@mwichmann
Copy link
Collaborator

mwichmann commented Jan 14, 2024

Just an update here, sorry it's been so long... I'm fiddling with Fortran module stuff which is a similar problem, though not identical... scanners are not properly accounting for the (optional) directory for these special files (.di or .mod) if it's supplied, so the paths end up wrong when searching for dependencies, and things go sideways. Testing a Fortran fix, need to add variantdirs into the test matrix first. It should apply to the D scanner as well, though there's a twist - a node reference for the directory is needed by the search function, and unlike Fortran, the D scanner is called without an environment reference, so it can't do env.fs.Dir(di_file_dir) to produce that. If that proves applicable, would have to do some rejiggering to make that happen, which isn't particularly hard, just some work and a bunch of testing to make sure such a change doesn't break things.

@mwichmann
Copy link
Collaborator

OK, I'm wrong... looks like the scanner does deduce the correct path:

scons: Building targets ...
looking for 'part.di' in 'hws' ...
looking for 'part.di' in 'build/include' ...
... FOUND 'part.di' in 'build/include'

and so the deps are correct:

  │ ├─┬build/include
  │ │ ├─┬build/include/main.di
  │ │ │ ├─hws/main.d
  │ │ │ ├─┬build/include/part.di
  │ │ │ │ ├─hws/parts/part.d
  │ │ │ │ └─/bin/gdc
  │ │ │ └─/bin/gdc
  │ │ └─┬build/include/part.di
  │ │   ├─hws/parts/part.d
  │ │   └─/bin/gdc

and the problem is just that that path doesn't make it into _DI_FLAGS when it's emitted to the command line (which is the approach you were following to solve this).

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

Successfully merging this pull request may close these issues.

2 participants