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

Compilation_db and TempFileMunge interference #4275

Closed
TZe-0xff opened this issue Dec 2, 2022 · 16 comments · Fixed by #4278
Closed

Compilation_db and TempFileMunge interference #4275

TZe-0xff opened this issue Dec 2, 2022 · 16 comments · Fixed by #4278

Comments

@TZe-0xff
Copy link
Contributor

TZe-0xff commented Dec 2, 2022

Describe the bug
Using compilation_db tool in a build where command line exceeds MAXLINELENGTH triggers the TempFileMunge mechanism (which creates temporary files to put command line into) leads to a unusable compilation_database.json (listed commands are not valid since relying on temporary files).

Required information

SCons by Steven Knight et al.:
        SCons: v4.3.0.559790274f66fa55251f5754de34820a29c7327a, Tue, 16 Nov 2021 19:09:21 +0000, by bdeegan on octodog
  • Version of Python
    Python 3.7.9 (tags/v3.7.9:13c94747c7, Aug 17 2020, 18:01:55) [MSC v.1900 32 bit (Intel)] on win32

  • Which python distribution if applicable (python.org, cygwin, anaconda, macports, brew,etc)

python.org

  • How you installed SCons
    pip

  • What Platform are you on? (Linux/Windows and which version)

Windows 10

  • How to reproduce your issue? Please include a small self contained reproducer. Likely a SConstruct should do for most issues.

SConstruct

env = Environment()
env['MAXLINELENGTH'] = 10
env.Tool('compilation_db')
env.Program("program", "src/main.c")
env.CompilationDatabase()

src/main.c

int main() { return 0; }
  • How you invoke scons (The command line you're using "scons --flags some_arguments")
    scons

  • Output

compile_commands.json

[
    {
        "command": "cl @C:\\Users\\...\\AppData\\Local\\Temp\\tmpmgrb17tp.lnk",
        "directory": "C:\\Data\\Workspaces\\Sublime\\compilation_db",
        "file": "src\\main.c",
        "output": "src\\main.obj"
    }
]
@acmorrow
Copy link
Contributor

acmorrow commented Dec 2, 2022

As noted in the discord discussion, my recommended way to handle this is to have the compilation database tool replace MAXLINELENGTH with a contextual generator which returns the user-provided value (if any) during normal expansion, but which returns a very large integer when expanded within the context of the compilation database logic which expands the command line.

@TZe-0xff
Copy link
Contributor Author

TZe-0xff commented Dec 5, 2022

I'm not sure what you mean by "contextual generator".
I managed to make things work (executed command uses temporary file while compilation database uses plain command lines) but I'm not convinced this is the right way to do it (cloning the Environment feels a bit heavy for this):

diff --git a/SCons/Tool/compilation_db.py b/SCons/Tool/compilation_db.py
index 9e72fe9a4..39d725722 100644
--- a/SCons/Tool/compilation_db.py
+++ b/SCons/Tool/compilation_db.py
@@ -32,6 +32,7 @@ which is the name that most clang tools search for by default.
 import json
 import itertools
 import fnmatch
+import sys
 import SCons
 
 from .cxx import CXXSuffixes
@@ -114,11 +115,13 @@ def compilation_db_entry_action(target, source, env, **kw):
     :param kw:
     :return: None
     """
-
+    cenv = env["__COMPILATIONDB_ENV"].Clone()
+    cenv['MAXLINELENGTH']=sys.maxsize
+    cenv = env["__COMPILATIONDB_ENV"]
     command = env["__COMPILATIONDB_UACTION"].strfunction(
         target=env["__COMPILATIONDB_UOUTPUT"],
         source=env["__COMPILATIONDB_USOURCE"],
-        env=env["__COMPILATIONDB_ENV"],
+        env=cenv,
     )
 
     entry = {

TZe-0xff pushed a commit to TZe-0xff/scons that referenced this issue Dec 5, 2022
TZe-0xff pushed a commit to TZe-0xff/scons that referenced this issue Dec 5, 2022
…ue just during compilation_db command substitution
@TZe-0xff
Copy link
Contributor Author

TZe-0xff commented Dec 5, 2022

After a bit of thought, we just need to change MAXLINELENGTH for the substitution, what do you think about it @acmorrow : TZe-0xff@dac7df3

@bdbaddog
Copy link
Contributor

bdbaddog commented Dec 5, 2022

The above is not a great solution, because the Environment() which you've cloned won't contain any changes made after the tool is initialized.
Things like CPPPATH, etc..

@TZe-0xff
Copy link
Contributor Author

TZe-0xff commented Dec 5, 2022

The above is not a great solution, because the Environment() which you've cloned won't contain any changes made after the tool is initialized. Things like CPPPATH, etc..

The code you mention is not the latest I would suggest (please have a look at my branch for a more light fix)

From my understanding, this code belongs to the action step and the strfunction call does the substitution directly. My new fix relies on that (and seems to work fine) : changing the MAXLINELENGTH just during this strfunction call.

@bdbaddog
Copy link
Contributor

bdbaddog commented Dec 5, 2022

You should not Clone() an environment in an Action..

@bdbaddog
Copy link
Contributor

bdbaddog commented Dec 5, 2022

As @acmorrow mentioned changing MAXLINELENGTH as a generator/python function would be the better way to do this.

@TZe-0xff
Copy link
Contributor Author

TZe-0xff commented Dec 5, 2022

Isn't the MAXLINELENGTH overload during the strfunction call the lightest way to do that ?
Otherwise, I don't see what you mean by this generator notion, could you point a analog mechanism ?

@TZe-0xff
Copy link
Contributor Author

TZe-0xff commented Dec 6, 2022

Obviously, I read about generators in documentation, but I may benefit from some guidance here.

@TZe-0xff
Copy link
Contributor Author

TZe-0xff commented Dec 7, 2022

Okay, I finally figured what you guys mean by using a generator (the SCons user guide was not helpful ...).
I put that in place but trying to discriminate the compilation_db substitution from the regular build one, I noticed that I have no difference in parameters (source, target, env) between calls to my generator.
Therefore my questions are :

  • do you think that compilation database is built only on a special call to scons ? (similar to Applelink usage of generator, based on a Environment variation)
  • if we need to set some variable in env["__COMPILATIONDB_ENV"] and remove it after the compilation_db expansion, why not directly backup and restore MAXLINELENGTH as suggested in the proposed commit ?

@TZe-0xff
Copy link
Contributor Author

TZe-0xff commented Dec 8, 2022

As suggested, I tried the env.CompilationDatabase(MAXLINELENGTH=sys.maxsize) but my output compile_commands.json still contains the temp file.
I think I can explain this because the compilation_db tool does not take into account its own environment, but rather stores the environment of object being emitted at each object emission and uses that afterwards to perform the command expansion.

@TZe-0xff
Copy link
Contributor Author

Just to be crystal clear, here is my proposal (this is typically more flexible than anything replacing MAXLINELENGTH at generation, intervention happens at very last possible moment and does not impact environment in which target will be built) :

diff --git a/SCons/Tool/compilation_db.py b/SCons/Tool/compilation_db.py
index 9e72fe9a4..370c68c3f 100644
--- a/SCons/Tool/compilation_db.py
+++ b/SCons/Tool/compilation_db.py
@@ -32,6 +32,7 @@ which is the name that most clang tools search for by default.
 import json
 import itertools
 import fnmatch
+import sys
 import SCons
 
 from .cxx import CXXSuffixes
@@ -114,13 +115,21 @@ def compilation_db_entry_action(target, source, env, **kw):
     :param kw:
     :return: None
     """
-
+    
+    # Disable potential TempFileMunge mechanism just for the substitution in following strfunction
+    # by raising MAXLINELENGTH to highest possible integer value
+    org_maxlinelength = env["__COMPILATIONDB_ENV"]['MAXLINELENGTH']
+    env["__COMPILATIONDB_ENV"]['MAXLINELENGTH'] = sys.maxsize
+    
     command = env["__COMPILATIONDB_UACTION"].strfunction(
         target=env["__COMPILATIONDB_UOUTPUT"],
         source=env["__COMPILATIONDB_USOURCE"],
         env=env["__COMPILATIONDB_ENV"],
     )
-
+    
+    # restore MAXLINELENGTH once substitution is done
+    env["__COMPILATIONDB_ENV"]['MAXLINELENGTH'] = org_maxlinelength
+    
     entry = {
         "directory": env.Dir("#").abspath,
         "command": command,

@bdbaddog
Copy link
Contributor

That's fine as long as you're not building in paralllel.. (only -j 1 is guaranteed to not step on another build step)
If you are, you can modify the environment's MAXLINELENGTH while an actual compile step is running.

I have another solution I'll be PR'ing shortly.

bdbaddog added a commit to bdbaddog/scons that referenced this issue Dec 11, 2022
…line), also extending serveral APIs to allow specifying an overrides dictionary which would override (at the last minute) any envvar or potentially env TARGET/SOURCE when subst is being called a subst() or subst_list(). Tests created. Docs still need to be updated.
@bdbaddog
Copy link
Contributor

The issue with this solution is if you build in parallel you may have a real compile step run inbetween changing and saving, and restoring MAXLINELENGTH.

I've just posted PR #4278 as another solution.

@TZe-0xff
Copy link
Contributor Author

You're absolutely right ! The original environment shall remain untouched at all (substitution) times. We actually use parallel build so I will be able to test your fix in another environment.
I saw you added tests on Subst, do you need me to add a test on compilation_db ?

@bdbaddog
Copy link
Contributor

You're absolutely right ! The original environment shall remain untouched at all (substitution) times. We actually use parallel build so I will be able to test your fix in another environment. I saw you added tests on Subst, do you need me to add a test on compilation_db ?

Good point. I have the original repro SConstruct from above, I'll wrap it in a test case, or add to an existing testcase possibly later today.

But, If you'd like to give it a try, you'll have to create a branch of my branch and then PR back to my branch.
I'd sugguest modifying an existing compilation_db test to cover this as well if you can. Ideally one which would run on all platforms. (That's the tricky part).
:)

TZe-0xff pushed a commit to TZe-0xff/scons that referenced this issue Dec 12, 2022
TZe-0xff pushed a commit to TZe-0xff/scons that referenced this issue Dec 12, 2022
bdbaddog added a commit to bdbaddog/scons that referenced this issue Dec 19, 2022
Added specific test for Compilation Database to reproduce issue SCons#4275
bdbaddog added a commit that referenced this issue Jan 2, 2023
Solution for Issue #4275 (Compilation db uses TEMPFILE generated command line instead of the plain command line)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants