Hi All, When throwing patches into emails, please add it as an attachment. It will be much more readable & quicker to understand your proposed solutions. The patch will be able to viewed in people's favorite diff viewers.
Thanks, Joey -----Original Message----- From: iotivity-dev-bounces at lists.iotivity.org [mailto:[email protected]] On Behalf Of Rees, Kevron Sent: Monday, June 15, 2015 2:49 PM To: iotivity-dev at lists.iotivity.org Subject: Re: [dev] HashTag: FixMe While on the topic of dependencies. The build should check whether the OS has the dependency installed first before downloading it. I used the following patch for cereal: diff --git a/extlibs/cereal/SConscript b/extlibs/cereal/SConscript index 946a483..cfe54fd 100644 --- a/extlibs/cereal/SConscript +++ b/extlibs/cereal/SConscript @@ -8,6 +8,7 @@ import os Import('env') src_dir = env.get('SRC_DIR') +conf = Configure(env) # In the pass, cereal library is in extlibs/cereal, according to external # library management rule, cereal should be put in extlibs/cereal/cereal. @@ -15,9 +16,19 @@ src_dir = env.get('SRC_DIR') # both places are handled. old = os.path.join(src_dir, 'extlibs', 'cereal', 'include') cur = os.path.join(src_dir, 'extlibs', 'cereal', 'cereal', 'include') +prefix = "/" + +try: + prefix = os.environ['PKG_CONFIG_SYSROOT_DIR'] +except: + prefix = "/" + +dist = os.path.join(prefix, 'usr', 'include', 'cereal') + +env = conf.Finish() # check 'cereal' library, if it doesn't exits, ask user to download it -if not os.path.exists(old) and not os.path.exists(cur): +if not os.path.exists(old) and not os.path.exists(cur) and not os.path.exists(dist): cereal_env = Environment(ENV = os.environ) c = cereal_env.Action(['git clone https://github.com/USCiLab/cereal.git cereal', 'cd cereal && git reset --hard 7121e91e6ab8c3e6a6516d9d9c3e6804e6f65245 && git apply ../../../resource/patches/cereal _gcc46.patch', @@ -38,4 +49,4 @@ if not os.path.exists(old) and not os.path.exists(cur): else: print 'Download cereal library complete' -env.AppendUnique(CPPPATH = [old, cur]) +env.AppendUnique(CPPPATH = [old, cur, dist]) On Mon, Jun 15, 2015 at 11:46 AM, Rees, Kevron <kevron.m.rees at intel.com> wrote: > On Mon, Jun 15, 2015 at 11:23 AM, Jon A. Cruz <jonc at osg.samsung.com> wrote: >> On 06/15/2015 11:10 AM, Rees, Kevron wrote: >>> Whether or not they are enabled by default is a different issue. I >>> don't care if they are enabled by default. I only care that I can >>> disable them. Currently I can't disable them from being built. >>> This means that I cannot avoid the extra dependencies the tests bring in. >>> To work around this, I have to patch scons like this: >> >> So I think there are two different issues here. >> >> >> First issue is tests requiring extra dependencies that the main code >> does not. Follow-ups would be to look into the details of the >> dependencies. These perhaps should not be required, could be checked >> for at configure time, etc. >> >> >> The second issue is disabling build of the unit tests altogether. >> That appears to be what the patch does (and is quite different from >> the first issue). > > It only disables them if TEST=0. You can make TEST=1 the default. > >> This really should not be done as it is a code-safety issue. >> Many of the same general reasons for using Gerrit and requiring all >> changes to get reviewed before committing apply here. If you work >> with >> TEST=0 causing unit tests to not be built, it is quite easy to >> accidentally break them and not notice. >> > > I understand from an iotivity development perspective that you might > want unit tests always built. But from a user perspective, this is > not acceptable. As a user of iotivity I may not care about the unit > tests and I don't want extra dependencies and unit test binaries in my > product that I don't need. > > >> For the latter case we do have Jenkins running with configurations >> that should catch broken tests, but that starts to suck resources >> that could have be avoided by catching theses issues at the >> developer's desk well before they commit. >> > > So make TEST=1 default. Problem solved. Sure, a developer might > compile with TEST=0 to disable the tests. He might also apply the > same hack that I did to disable the building of the unit tests > altogether. There is a trade-off here. If you want to coddle > developers and make sure they always build and run unit tests, then > you need to remove "if TEST=='1'" altogether. If you want to make > life easier for users who don't want the extra dependencies, keep > TEST==1, make it default, but do the "right thing" TM and allow users > to turn them off if they don't care about the tests (which they should > probably never see anyway). > >> So to sum up, it appears you asked a "how do I do such-and-such" >> without covering the "why I want to do such-and-such". If you are >> asking due to the first issue (which your later mail seems to >> indicate) then the proper solution would probably be for us to fix >> the broken tests so that building all tests won't need to be disabled. >> > Why do I want the option to disable the unit tests? This is a valid > question. As a user I don't care about the unit tests. I don't want > to have to build or install gtest or hippomocks in order to build what > I do care about: iotivity. > > I know a lot of open source projects that do unit testing. I can't > think of any that force me to build them/run them as a user. > >> -- >> Jon A. Cruz - Senior Open Source Developer Samsung Open Source Group >> jonc at osg.samsung.com _______________________________________________ >> iotivity-dev mailing list >> iotivity-dev at lists.iotivity.org >> https://lists.iotivity.org/mailman/listinfo/iotivity-dev _______________________________________________ iotivity-dev mailing list iotivity-dev at lists.iotivity.org https://lists.iotivity.org/mailman/listinfo/iotivity-dev
