This is an archive of the discontinued LLVM Phabricator instance.

[gn build] improve write_cmake_config to be truthy and exception friendly
Needs ReviewPublic

Authored by zhongxiao.yzx on Sep 18 2021, 1:40 AM.

Details

Reviewers
thakis
mantognini
Summary
The rule of python for strings is that an empty string is
considered False, a non-empty string is considered True.
It means '0','false' and many other string value will be
treat "true" which should be "false". "distutils.util.strtobool"
can help use to solve the problem to much extent.

Besides, in order to substiute #cmakedefine var with value,
it needs to access "values[key]" which will raises a "KeyError"
exception if searching for a key that does not exist. "values.get(key)"
can help use to avoid "KeyError" exception in a simply way.

Diff Detail

Event Timeline

zhongxiao.yzx created this revision.Sep 18 2021, 1:40 AM
zhongxiao.yzx requested review of this revision.Sep 18 2021, 1:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2021, 1:41 AM
  • reorder the import statement
  • remove unused pass statement of try-except
mantognini resigned from this revision.Sep 18 2021, 5:40 AM

Can you explain the motivation behind this?

Besides, in order to substiute #cmakedefine var with value,
it needs to access "values[key]" which will raises a "KeyError"
exception if searching for a key that does not exist.

That part is intentional, to make sure the list of keys passed to the script matches exactly the list of keys needed in the cmake file.

Can you explain the motivation behind this?

Besides, in order to substiute #cmakedefine var with value,
it needs to access "values[key]" which will raises a "KeyError"
exception if searching for a key that does not exist.

That part is intentional, to make sure the list of keys passed to the script matches exactly the list of keys needed in the cmake file.

It’s more convenient to make an appointment in advance:

Taking the following example, the project needs to define the "header.h.in" with the following contents

  • header.h.in *
#cmakedefine Key-1
#cmakedefine Key-2
...
#cmakedefine Key-i
...
#cmakedefine Key-j
...
#cmakedefine Key-n
  • write_cmake_config_key represents "the list of keys passed to the script", it's the KYE=VALUE should be provided while invoking write_cmake_config.py
write_cmake_config.py  -o  header.h  header.h.in  ${write_cmake_config_key}
  • header_key represents "the list of keys needed in the header file." which corresponds to " the list of keys needed in the cmake file"

My intents to replace "values[keys]" with "values.get()" are as follows:

1. facilitate write_cmake_config.py invoking with default values for optional keys.

There are many keys in "header.h.in", in order to generate the "header.h", we may only
want to provide the values for keys those we want to overwrite the default values
rather than provide all the keys defined in "header.h.in" even if most of them are
duplicated with default values
we can invoke write_cmake_config.py only define the Value-i, Value-j leaving other keys with default values

write_cmake_config.py  -o  header.h  header.h.in  Key-i=Value-i  Key-j=Value-j

Yes, of course, we can provide all the values for keys defined in "header.h.in"

2. keep compatible with config_file in CMakeLists.txt

The behaviours of config_file in the CMake build system is the same as described above.
Furthermore, with the help of "unused_values checking", It is also guaranteed that the list
of keys passed to the script matches exactly the sub-list of keys needed in the "header.h.in"

3. keep friendly to be integrated

"write_cmake_config.py" works integrated into gn build system, the KeyError exception is
not friendly enough to be caught in gn system.

"Ping."

@thakis please take a look and give your suggestion
or can you help me to commit it if it's acceptable
(I have not got the right to commit yet!)

alexfh removed a reviewer: alexfh.Oct 1 2021, 4:14 PM
thakis added a comment.Oct 4 2021, 8:58 AM

I still don't understand what the effect of this is or what the motivation behind is, sorry :(

Hi, all
As explained in my reply, it's mainly intended to improve the judgment for
the "Truthy or Falsy" Values;
meanwhile, keep the behavior to be consistent with what the "config_file"
does.
Because, it's better to fill with the default value rather than the
exception, and to make
the string literal "true", "false" or "0" etc. to be judged as CMake does
in many cases.

If it does not make any sense, can you give me some advice?

Nico Weber via Phabricator <reviews@reviews.llvm.org> 于2021年10月4日周一
下午11:58写道:

thakis added a comment.

I still don't understand what the effect of this is or what the motivation
behind is, sorry :(

CHANGES SINCE LAST ACTION

https://reviews.llvm.org/D110019/new/

https://reviews.llvm.org/D110019