Page MenuHomePhabricator

eolian_gen: Add support to response files.
Needs ReviewPublic

Authored by jptiz on Aug 11 2020, 1:35 PM.

Details

Summary

In some platforms such as Windows, there are cases when command line
length is too short even to run Eolian tests (depending on which
directory EFL repository is cloned into). This commit makes enables the
usage of response files, which contains each of the args that would go
into command line splitted one per line.

Ref T8768

Test Plan

meson test -C build --print-errorlogs

Diff Detail

Repository
rEFL core/efl
Branch
devs/jptiz/eolian-gen-phab
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17326
Build 11589: arc lint + arc unit
jptiz created this revision.Aug 11 2020, 1:35 PM
jptiz requested review of this revision.Aug 11 2020, 1:35 PM
jptiz edited the summary of this revision. (Show Details)Aug 11 2020, 1:38 PM
jptiz edited the summary of this revision. (Show Details)
jptiz updated this revision to Diff 31023.Aug 11 2020, 1:43 PM

Small enhancement for -r's description in usage

vtorri added inline comments.Aug 11 2020, 1:51 PM
src/bin/eolian/main.c
497

we don't use camel case for the name of types, we add _ to separate parts. Like :

Cmd_Line_Options

src/lib/eina/eina_file_win32.c
109 ↗(On Diff #31022)

maybe another patch for this one ? The code is not wrong as INVALID_FILE_ATTRIBUTES is equal to 0xffffffff, but you're right, better using this name.

jptiz added inline comments.Aug 11 2020, 1:53 PM
src/bin/eolian/main.c
497

Oh, thanks. Gonna fix that.

src/lib/eina/eina_file_win32.c
109 ↗(On Diff #31022)

Hm, indeed, going to split them then.

jptiz updated this revision to Diff 31024.Aug 11 2020, 1:58 PM

Fix CmdLineOptions case to Cmd_Line_Options and undo
INVALID_FILE_ATTRIBUTES change (which will be splitted into another patch).

jptiz updated this revision to Diff 31025.Aug 11 2020, 2:02 PM

Fixed INVALID_FILE_ATTRIBUTES in the right place.

jptiz marked 2 inline comments as done.Aug 11 2020, 2:02 PM
vtorri added inline comments.Aug 14 2020, 8:20 AM
src/bin/eolian/main.c
502

you can memset to 0 options and set to EINA_TRUE and 1 the 2 relevant fields

667

is it really necessary to have this function if it called once ?
I would do :

/* init options */
memset(&options, 0, sizeof(options));
options.scan_system = EINA_TRUE;
options.pret = 1;
jptiz added inline comments.Aug 14 2020, 9:26 AM
src/bin/eolian/main.c
502

What do you think about options = {0} in that case?

667

It was for an organization matter, but since the initialization is pretty straight-forward it makes sense to me to "inline" it here.

jptiz updated this revision to Diff 31030.Aug 14 2020, 9:35 AM

Inline options initialization.

q66 accepted this revision.Aug 17 2020, 1:36 AM
This revision is now accepted and ready to land.Aug 17 2020, 1:36 AM

This does not apply to latest HEAD anymore, please rebase.

jptiz updated this revision to Diff 31046.Aug 19 2020, 10:39 AM

Rebase to correct commit

This does not apply to latest HEAD anymore, please rebase.

Woops, totally forgot this was on top of another commit.

Should be fixed now.

Applies now, but shows errors when building:

[1/3252] Compiling C object 'src/bin/eolian/70b716a@@eolian_gen@exe/main.c.o'
../src/bin/eolian/main.c: In function ‘_read_options_from_response_file’:
../src/bin/eolian/main.c:524:22: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
  524 |    for (int i = 0; i < file_length; ++i)
      |                      ^
../src/bin/eolian/main.c:543:22: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
  543 |    for (int i = 0; i < file_length; ++i)
      |                      ^
../src/bin/eolian/main.c:548:23: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
  548 |              while (i < file_length && contents[i] == '\0')
      |                       ^
../src/bin/eolian/main.c:553:20: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
  553 |              if (i >= file_length)
      |                    ^~
stefan_schmidt requested changes to this revision.Aug 19 2020, 10:51 PM
This revision now requires changes to proceed.Aug 19 2020, 10:51 PM

Just my two cents: I think the tests should unconditionally run with the respones files, and not only if PATH_MAX is too small, this way we can ensure that the respone file is tested on any maschine that runs the tests.

Its even more than that. When running the test suite I get the following:

../src/tests/eolian/eolian_generation.c: In function ‘_eolian_gen_execute’:
../src/tests/eolian/eolian_generation.c:127:67: error: ‘type’ undeclared (first use in this function)
  127 |         int failed = _create_response_file(rsp_path, eo_filename, type, output_filename);
      |                                                                   ^~~~
../src/tests/eolian/eolian_generation.c:127:67: note: each undeclared identifier is reported only once for each function it appears in
<command-line>: warning: ‘%s’ directive output may be truncated writing up to 4095 bytes into a region of size 4034 [-Wformat-truncation=]
<command-line>: note: in definition of macro ‘EOLIAN_GEN’
../src/tests/eolian/eolian_generation.c:123:9: note: ‘snprintf’ output between 64 and 4159 bytes into a destination of size 4096
  123 |         snprintf(command, PATH_MAX,
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
  124 |                  EOLIAN_GEN" -r \"%s\"",
      |                  ~~~~~~~~~~~~~~~~~~~~~~~
  125 |                  rsp_path);
jptiz added a comment.EditedAug 20 2020, 12:44 AM

Applies now, but shows errors when building:

[1/3252] Compiling C object 'src/bin/eolian/70b716a@@eolian_gen@exe/main.c.o'
../src/bin/eolian/main.c: In function ‘_read_options_from_response_file’:
../src/bin/eolian/main.c:524:22: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
  524 |    for (int i = 0; i < file_length; ++i)
      |                      ^
../src/bin/eolian/main.c:543:22: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
  543 |    for (int i = 0; i < file_length; ++i)
      |                      ^
../src/bin/eolian/main.c:548:23: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
  548 |              while (i < file_length && contents[i] == '\0')
      |                       ^
../src/bin/eolian/main.c:553:20: warning: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
  553 |              if (i >= file_length)
      |                    ^~

*Facepalm* My bad, totally didn't notice that. Gonna fix that.

Its even more than that. When running the test suite I get the following:

../src/tests/eolian/eolian_generation.c: In function ‘_eolian_gen_execute’:
../src/tests/eolian/eolian_generation.c:127:67: error: ‘type’ undeclared (first use in this function)
  127 |         int failed = _create_response_file(rsp_path, eo_filename, type, output_filename);
      |                                                                   ^~~~
../src/tests/eolian/eolian_generation.c:127:67: note: each undeclared identifier is reported only once for each function it appears in
<command-line>: warning: ‘%s’ directive output may be truncated writing up to 4095 bytes into a region of size 4034 [-Wformat-truncation=]
<command-line>: note: in definition of macro ‘EOLIAN_GEN’
../src/tests/eolian/eolian_generation.c:123:9: note: ‘snprintf’ output between 64 and 4159 bytes into a destination of size 4096
  123 |         snprintf(command, PATH_MAX,
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~
  124 |                  EOLIAN_GEN" -r \"%s\"",
      |                  ~~~~~~~~~~~~~~~~~~~~~~~
  125 |                  rsp_path);

Oh, right. Well, gonna test a little better before updating.

jptiz updated this revision to Diff 31049.Aug 20 2020, 1:42 AM

Fix wrong cherry-pick resolution (i.e. use options again instead of type, which was from another commit that is not on upstream).

jptiz added a comment.Aug 20 2020, 2:00 AM

Just my two cents: I think the tests should unconditionally run with the respones files, and not only if PATH_MAX is too small, this way we can ensure that the respone file is tested on any maschine that runs the tests.

You mean adding a test for that or actually changing all tests to use a response file?

stefan_schmidt requested changes to this revision.Aug 20 2020, 3:12 AM

While the tests build error is gone the different signedness warnings are still there

This revision now requires changes to proceed.Aug 20 2020, 3:12 AM
jptiz updated this revision to Diff 31051.Aug 20 2020, 3:56 AM

Forgot to add int -> size_t changes.

jptiz updated this revision to Diff 31052.Aug 20 2020, 4:29 AM

Fix issue with rsp file path having the same size as final command-line string.

jptiz planned changes to this revision.Aug 20 2020, 4:30 AM

Going to add a response-file test.

jptiz updated this revision to Diff 31138.Sep 14 2020, 12:38 PM

Actually execute eolian_gen in _eolian_gen_execute_rsp function.

jptiz updated this revision to Diff 31143.Sep 16 2020, 10:14 AM

Add a small comment about response_file test.

jptiz added a comment.Sep 17 2020, 1:22 PM

This is ready for review again.