#953 Add response file handling

[bluehazzard]

This Patch adds response files if the command line is to long. This is needed on windows, becaise the command line has a length limit.

The Patch adds two options to the compiler options dialog:
1) Enable response files
2) Response file command line length limit

If Option 1) is enabled and the command line is longer then the Option 2) then the command is split into the maximal length and the too long part is stored in a respFile in the object file directory.
The command line to the compiler is then modified to contain the "@" plus the path to the response file.

One thing is still missing: The build percentage is not working, because i add some log information to the build log. Also I would like to add some information to the build log html file. I will fix this bugs, if the approaci here is ok...

please give some feedback.

A script to generate a project with to long names:

activeProject <- GetProjectManager().GetActiveProject();

filename <- _T("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA.cpp");

for(local i = 0; i < 2000; i+=1)
{
    local thisFileName = _T(i.tostring()) + _T("AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA.cpp");
    local path =  activeProject.GetCommonTopLevelPath() + _T("src") + wxFILE_SEP_PATH + thisFileName;
    IO.WriteFileContents(path , _T("void func") + _T(i.tostring()) + _T("() {}") );
    print("create file: " + path)

    GetProjectManager().AddFileToProject(path, activeProject, 0);
}

activeProject.Save();

First create a project, and a "src" sub directory. Then run the above script and save the project. On windows you will hit the max command line length problem on linking...

related forum link: forum

[Teodor Petrov]

It would be easier to follow if you post a branch on github.
Also it is best to not use size_t or any other unsigned type if you don't have to. Unsigned types are really dangerous for anything but flags.

Adding options for this is wrong. Just make it work out-of-the box. You can find the limits for all the OSes we support. Put them in there and don't make the user choose.

How many compilers have you tried? Does it work the same with all of them?

[Teodor Petrov]

BTW: There is some issue logged for this somewhere...

[bluehazzard]

it would be easier to follow if you post a branch on github.

https://github.com/bluehazzard/codeblocks_sf/tree/fix/ticket/953/1

Adding options for this is wrong. Just make it work out-of-the box. You can find the limits for all the OSes we support. Put them in there and don't make the user choose.

I have found a lot different numbers (and ranges) for BSD Linux MacOS and windows.... Why not simply set a reasonable limit (probably 32k, the lowest i have found for windows, but for windows i have also found 128 xD ) and let the user decide? In the best case he never touches the option in the worst case he has an option....

How many compilers have you tried? Does it work the same with all of them?

I personall use and have set up and tested only gcc, but according documentation the same should work for msvc, bcc, clang and intel. I have not checked for more....

BTW: There is some issue logged for this somewhere...

will search for it

[bluehazzard]

related ticket https://sourceforge.net/p/codeblocks/tickets/23/

[Teodor Petrov]

I don't know why people ignore the post of mr Chen I've linked on the forum. This is the only reputable source you should listen to. He obviously knows what he is talking about. The same cannot be said about other people on the internet . :(

I dislike options. They make the code harder to understand and harder to test. The only option I think we must have is "always use responce files". What happens if you implement it and use such version to build codeblocks and contribs? Would it work?

This is a minimal test you should do before this becomes acceptable for trunk.

[bluehazzard]

In this official documentation of microsoft is a number of 127 https://docs.microsoft.com/en-us/cpp/build/reference/at-specify-a-compiler-response-file?view=vs-2019

Here is a probably outdated list of unix oses with different limits
https://www.in-ulm.de/~mascheck/various/argmax/

so it is not that easy...

[Teodor Petrov]

For Windows read and use what Chen say, this documentation is probably outdated or too restricted.

For linux this seems inf, but you could cap it at 256 * 1024, just in case.
For the rest using ARG_MAX seems good enough or ARG_MAX/2. The correct header or the value to use could be detected at configure time.

As I've said before - having an option with a value is the wrong solution!

[bluehazzard]

For the rest using ARG_MAX seems good enough or ARG_MAX/2. The correct header or the value to use could be detected at configure time.

for this i need help... I have no idea how to use constants defined at configure time in a software... Over -D compiler flag?

[Teodor Petrov]

Some of these might help https://autotools.io/autoconf/index.html
The idea is to detect which macro you have at autotools configure time.

[bluehazzard]

Ok, i use some default defines for this
this patch is not tested on linux, i will try to do that as soon as possible

i have to use hashes for the name generation, because i got name collisions. Should the filename of the file generated with the response file be in the response file name?

I still have to clean up the code, i just wanted confirmation that my approach is ok

This compiles fine the whole codeblocks project (the limit on windows is set to 200 in thsi patch). But it fails with the resource compilation because winres does not understand the @ command line option. I the real world i do not think that we have a problem with this, because the commandlne probably gets only this long during linking...

github is updated.

NOTE: I STILL NEED TO CLEAN UP THE CODE.... I just want to know if the approach with the defines is ok...

[Teodor Petrov]

I don't get why you need the hashes. If you use something like .objs/targetName/exename.exe.response you should have no problem. If you want to generate a temp file there are APIs for this. https://docs.wxwidgets.org/trunk/classwx_file_name.html#a6c56af101aea8c3d98c693811a44db69

[bluehazzard]

The files have to be created in advance (all files in one time, then the compiling, atleast with my approach here), so if one file has the same name as a other it wont work anymore. I tested a lot name combinations, but in the end, the only thing that worked without flaws (building the codeblocks workspace) was hashing. Also it was faster then concatenating strings (i did not measure it, only by feeling)

[Teodor Petrov]

Why does creating .objs/targetName/targetOutputName.extName.respone won't work for codeblocks? Which file breaks it?

(please don't say it was faster if you have no measurement info, it is lame)

[Teodor Petrov]

Also I suppose you can log a job item to create the file just before the execution of the command. Or at the execution of the command. But I'm not an expert on the compiler plugin.

[bluehazzard]

next iteration:
The problem were project where a target name is the same as a cpp file and output file.
I fixed this by adding "link" to the linking response files

Also I suppose you can log a job item to create the file just before the execution of the command.

I think this is the better place. This should be in the command line generation and not the command execution, because this is compiler (executable) specific.

At the moment i really have not time to extend the work here. I purpose this patch to commit it. I should only affect a small amount of people on windows, but there are actually at least two people with the problem in the forum, so it urges enough to push a fix

[Teodor Petrov]

1. There is no comment what is COMPILER_ONLY_NOTE_ID_LOG
2. Why are you using a define which would be set to a local variable 3 lines later. Just set the local variable.
3. What does safety margin means? Why would you do this?
4. Add a link/s with info about various limits.
5. No alignments please, they are stupid and cause problems when someone later modifies this code
6. Add comments for the parameters of CheckForToLongCommandLine
7. Change them to something more meaningful. Array is too generic.
8. Why do you assume that that the compiler command would be the name of the executable?
9. You know that wxFileName is highly inefficient and should be used with care, don't you? It seems you don't, because you're calling GetFullPath 6 times.
10. What does " // escaping Needed for windows" means? Clarify.
11. Use camelCase and no underscores. (compiler_cmd)

[bluehazzard]

Ok, i tried to honor all your comments.

Why are you using a define which would be set to a local variable 3 lines later. Just set the local variable.

As far as i understand with this you can override the limit during compile time. For example with configure and honor the ARG_MAX during compiling

thank you fro the comments.

[Teodor Petrov]

BTW: Again it would be easier if you do pull requests on github. This way I won't need to remember all the comments I've made in the previous iteration. You can do a rebase/squash at the end. We won't use github for merging anyway.

1. "too long", not "to long". This mistake is present at multiple places.
2. line too long "const wxString COMPILER_ONLY_NOTE_LOG(..." put the comment above it. Change it to doxygen comment. Use /// and not /* */
3. Put the # at the start of the line. Don't indent the code inside the ifdef
4. There is useless empty line
5. I'm not sure allowing the override of CB_COMMAND_LINE_MAX_LENGTH on the command line is really useful. If someone needs to make a package for some strange OS/distro it is the same if they have to modify the code or the build system. I suppose that modifying the build system would be even harder.
6. Don't do concatenation with translateable string. Use string formatting.
7. rfind + find combination is not much better than the previous approach. There is no comment why you want to do it.
8. "an pre-appended" -> "a pre-appended"
9. Stop this alignment nightmare. I don't think putting the types is useful for doxygen.
10. This is my most serious complaint about this - why are you doing complex/smart thing with the command line length? I'm talking about the variable splitting. To me the logic should be "if the line length passes the limit replace the command with executable-path @response-file". Simple, robust and it would be easier for people to see all the options in a single place.

[bluehazzard]

BTW: Again it would be easier if you do pull requests on github.

done

There is useless empty line

please point out in the github pull request.

I'm not sure allowing the override of CB_COMMAND_LINE_MAX_LENGTH on the command line is really useful. If someone needs to make a package for some strange OS/distro it is the same if they have to modify the code or the build system. I suppose that modifying the build system would be even harder.

You requested this 11 comments up. Please decide...

rfind + find combination is not much better than the previous approach. There is no comment why you want to do it.

This code is executed if a command is called that has a longer limit then the command line limit of the os. I think searching for some string is not the longest time it takes. in this case you are even writing a file to disk, that is probably a lot slower then this search... Anywhay, see bot why i do this and decide if it is needed or not

Stop this alignment nightmare. I don't think putting the types is useful for doxygen.

please be more precise. Point to the line or block. i can not read your mind.... For me alignment makes the code a lot more readable. The argumetn it makes editing the code harder is some subjective thing, like saying it makes the code more readable. For me it is easier to edit the code. I have some vision problems, and cramped code is really hard to read for me. Spaces, alignments and empty lines easy the reading for me a lot...

This is my most serious complaint about this - why are you doing complex/smart thing with the command line length? I'm talking about the variable splitting. To me the logic should be "if the line length passes the limit replace the command with executable-path @response-file". Simple, robust and it would be easier for people to see all the options in a single place.

Because i do not know the compiler command, executable-path... I can guess it with spaces, but when the compiler command has spaces this is a really bad guess... You can strict the user to compiler commands without spaces, but i do not think this is a good idea...
I do not know the compiler command, because this whole command generation is a rabbit hole. Feel free to search for it and implement it.. But i think you have no time to do it and i have also not the time for it.
There are people out there whaiting for this to work. "Perfect is the enemy of good" and perfect in our case means never implemented or fixed....

[Teodor Petrov]

There are people out there whaiting for this to work. "Perfect is the enemy of good" and perfect in our case means never implemented or fixed....

What? This argument is invalid. But I guess I'll leave you do whatever you like. I guess we'll run in circles and I'll never be happy. So push whatever you like.

[bluehazzard]

i am open for discussions, hints, formatting requests and so on as long as they are detailed (like how should i know what identation do you not like, or what empty line is to much...), I am just fustrated about the running in circles... 11 Comments above you say we need to do this on configure, then we should not do this in configure and then whay do you not do this compelately different.... It is fustrating...

[Teodor Petrov]

:shrug:

[bluehazzard]

Ok, i pushed this to trunk
https://sourceforge.net/p/codeblocks/code/12169/

i have tested it on my linux (Mint 19 -> Ubuntu 18.04 ) machine and the actual limit for excev was 131072 (half of preasumly BSD) so i used this limit.

I leave this ticket open for the moment, if someone has more or better ideas to make this.

thank you again, for the discussion and the help!

[Teodor Petrov]

@bluehazzard: If you don't intend to do any more work on this I think it should be closed.