extedended main SConscript to support building executables of C++ cod… by iagaponenko · Pull Request #375 · lsst/qserv

iagaponenko · 2018-02-16T00:33:10Z

e, added a demo application which tests if versions of the Gpoogle Protobuf headers and libraries match

…e, added a demo application which tests if versions of the Gpoogle Protobuf headers and libraries match

andy-slac

This is not what I expected 😑 Let me think about how to do it right, I'll probably make another commit on your branch tomorrow.

andy-slac · 2018-02-16T00:57:24Z

core/modules/tests/SConscript

+standardModule(env, exclude=exclude, unit_tests="")
+
+Return('module_build_targets')
+s


Oops! I'll fix this.

andy-slac · 2018-02-16T00:59:18Z

core/modules/tests/SConscript

+
+standardModule(env, exclude=exclude, unit_tests="")
+
+Return('module_build_targets')


We don't use Return, everything is passed via env

I thought that a module's SConscript is a function which returns a value of a locally defined (within the called module) variable which is inspected by the main SConscript. That (returned) list is used on each iteration of the main SConscript's look over modules. Are you suggesting I should inject that variable into the global 'env'?

andy-slac · 2018-02-16T01:04:37Z

core/modules/tests/SConscript

+                "protobuf",
+                "log",
+                "log4cxx"])
+    exclude.append(f)


We should try to make this generic and move to standardModule()

Hence, the idea is to add an optional parameter to "standardModule" to allow passing a list of module's binary products (along with their dependencies), someting like:
standardModule(binaries=[
{"tests_app.cc": ["qserv_common","util","log4cxx"]},
{"tests_somethingelse.cc":[...]),,,],

andy-slac · 2018-02-16T01:06:01Z

core/modules/tests/SConscript

+# SConscript. Also add the source file into a list of sources to be excluded
+# from the shared library product.
+
+env.Append(LIBPATH=['$build_dir'] + env['LIBPATH'])


That changes global environment which you don't want to do.

Should I then pass this as a parameter to the Program target when doing?
env.Program(...LIBPATH=['$build_dir'] + env['LIBPATH']...)

Okay, I've deleted a line of code which modified LIBPATH in the global environment and passed it to each env.Program() invocation as a parameter. And this works.

…late code into function standardModule. This should simplify module-level SConscript file, and it will also make them more robust

iagaponenko · 2018-02-16T04:26:25Z

@andy-slac okay, I've made an improved version of the build system as per your suggestion. NOw it should be trivial to add C++ binaries from the module's SConscriot files - just a dictionary of C++ file names (keys) and list of the corresponding libraries for dependencies It's also more robust compared with the prior version. Have a look at SConscript file of module "tests" as an example. Could you have a look at it please? Thanks!

andy-slac

Looks mostly OK, see comments below.

andy-slac · 2018-02-16T05:03:20Z

core/modules/SConscript

                then use this parameter to pass list (or space-separated
                string) of application names. Can be empty string or list to
                not run any unit tests.
+    @param bin_cc_targets: a dictionary defining whgich binary products (not


whgich -> which
Not shure targets has a meaning here. The key in dict is a name of the source, not target?

I'll come up with a better name

andy-slac · 2018-02-16T05:14:08Z

core/modules/SConscript

+    # library or unit tests
+    exclude_cc_files = []
+    if exclude is not None:
+        exclude_cc_files = exclude


exclude_cc_files += exclude, otherwise you are potentially modifying exclude too

Agreed. My Python is rusty :-)

andy-slac · 2018-02-16T05:18:52Z

core/modules/SConscript

+    if exclude is not None:
+        exclude_cc_files = exclude
+    if bin_cc_targets is not None:
+        for f in bin_cc_targets: exclude_cc_files.append(f)


exclude_cc_files += bin_cc_targets.keys()?

andy-slac · 2018-02-16T05:21:31Z

core/modules/SConscript

                string) of application names. Can be empty string or list to
                not run any unit tests.
+    @param bin_cc_targets: a dictionary defining whgich binary products (not
+                the unit tests should be built out of C/C++ files. Each key


Opening parenthesis on previous line is not closed, I guess it should go after "tests".

andy-slac · 2018-02-16T05:25:40Z

core/modules/SConscript

+    # inject binary targets built of the optionally provided C/C++ files
+    if bin_cc_targets is not None:
+        for f,dependencies in bin_cc_targets.items():
+            source = os.path.basename(str(f))


Why do you need basename for source?

Agreed. No need. I will eliminate it.

andy-slac · 2018-02-16T05:27:13Z

core/modules/SConscript

+                target=target,
+                source=[source],
+                LIBS=dependencies,
+                LIBPATH=['$build_dir'] + env['LIBPATH'])


Looks a bit complicated, I'd think this should work:

for f, dependencies in bin_cc_targets.items(): p = env.Program(f, LIBS=dependencies, LIBPATH=['$build_dir'] + env['LIBPATH'])

I will give it a try, and if it works then I'll make mods to the code.

andy-slac · 2018-02-16T05:34:05Z

core/modules/tests/SConscript

+bin_cc_targets = {}
+path = "."
+for f in env.Glob(os.path.join(path, "tests_*.cc"), source=True, strings=True):
+    bin_cc_targets[f] = ["qserv_common","util","protobuf","log","log4cxx"]


Don't overdo it. Normally all "test*.cc" files are (unit) tests and we don't want their binaries installed so don't try to make all test_*.cc into true binaries. I'd probably give it different name, not starting with "test".

I believe you can also write:

bin_cc_targets[f] = "qserv_common util protobuf log log4cxx"

Perhaps we should come up with some naming convention for the binaries. Something like: qserv--.cc. For instance my only app module tests should be renamed from "tests_protobuf_version.cc" into "qserv-tests-protobuf.cc". And I agree about avoiding using "test*" in apps names". I just wanted to see if my implementation of standardModule would work. And thanks for the advice to replace a list of dependencies with a string. I'll give it a try.

iagaponenko · 2018-02-16T08:06:33Z

@andy-slac thank you for reviewing the code and making good suggestions! I've just committed a few more modification to the code as per your suggestions.

extedended main SConscript to support building executables of C++ cod…

86d2b63

…e, added a demo application which tests if versions of the Gpoogle Protobuf headers and libraries match

andy-slac reviewed Feb 16, 2018

View reviewed changes

iagaponenko added 3 commits February 16, 2018 04:17

improved version of the build tool which moves the common and boilerp…

e15005e

…late code into function standardModule. This should simplify module-level SConscript file, and it will also make them more robust

fixed a small typo

cd5b0db

fixed minor typos and indentetions

ba1bfb2

andy-slac approved these changes Feb 16, 2018

View reviewed changes

made changes as requested by the reviewer

6860c84

iagaponenko merged commit df45244 into master Feb 16, 2018

ktlim deleted the tickets/DM-13574 branch August 23, 2018 00:24


		standardModule(env, exclude=exclude, unit_tests="")

		Return('module_build_targets')

Conversation

iagaponenko commented Feb 16, 2018

Uh oh!

andy-slac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iagaponenko Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iagaponenko commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andy-slac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iagaponenko commented Feb 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iagaponenko Feb 16, 2018 •

edited

Loading

iagaponenko commented Feb 16, 2018 •

edited

Loading