| Message ID | 20200306172030.27791-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Fri, Mar 06, 2020 at 05:20:30PM +0000, Kieran Bingham wrote: > The control headers are generated automatically by parsing our YAML > descriptions, and creating the control headers. > > The headers for the controls can be used directly from within libcamera > internal pipeline handlers and core components, but there is no link to > ensure that the headers are generated before they are used. > > As part of updating controls to support properties, the commit > f870591a9bf5 ("libcamera: properties: Add location property") also > inadvertently moved the generated headers out of the libcamera_api > dependency generation. > > This allowed a race condition to occur in builds where objects are > attempted to be built before the API definitions had been generated. > > Declare a dependency on the headers for libcamera to ensure that they > are built before compiling any object within the libcamera library, and > re-introduce the headers to the libcamera_api variable. This is bundling two changes in one patch, and given the very low probability of hitting the race, and unsuccessful past attempts to fix it, I would prefer decoupling the two. Would you be fine with the first patch carrying the libcamera_api change only, with the Fixes: line, and the above paragraph removed, and a second patch that adds the declary_dependency ? It would allow testing the two independently. > Fixes: f870591a9bf5 ("libcamera: properties: Add location property") > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/meson.build | 2 ++ > src/libcamera/meson.build | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > index f47c583cbbc0..88edf620f69e 100644 > --- a/include/libcamera/meson.build > +++ b/include/libcamera/meson.build > @@ -44,6 +44,8 @@ foreach header : control_source_files > install_dir : join_paths('include', include_dir)) > endforeach > > +libcamera_api += control_headers > + > gen_header = files('gen-header.sh') > > libcamera_h = custom_target('gen-header', > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index 88658ac563f7..cd95fa11534a 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -97,6 +97,7 @@ libcamera_deps = [ > cc.find_library('dl'), > libudev, > dependency('threads'), > + declare_dependency(sources : [control_headers]) > ] > > libcamera_link_with = []
Hi Laurent Sorry for HTML reply, but I'm phone only at the moment. On Fri, 6 Mar 2020, 20:35 Laurent Pinchart, < laurent.pinchart@ideasonboard.com> wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Mar 06, 2020 at 05:20:30PM +0000, Kieran Bingham wrote: > > The control headers are generated automatically by parsing our YAML > > descriptions, and creating the control headers. > > > > The headers for the controls can be used directly from within libcamera > > internal pipeline handlers and core components, but there is no link to > > ensure that the headers are generated before they are used. > > > > As part of updating controls to support properties, the commit > > f870591a9bf5 ("libcamera: properties: Add location property") also > > inadvertently moved the generated headers out of the libcamera_api > > dependency generation. > > > > This allowed a race condition to occur in builds where objects are > > attempted to be built before the API definitions had been generated. > > > > Declare a dependency on the headers for libcamera to ensure that they > > are built before compiling any object within the libcamera library, and > > re-introduce the headers to the libcamera_api variable. > > This is bundling two changes in one patch, and given the very low > probability of hitting the race, and unsuccessful past attempts to fix > it, I would prefer decoupling the two. > > Would you be fine with the first patch carrying the libcamera_api change > only, with the Fixes: line, and the above paragraph removed, and a > second patch that adds the declary_dependency ? It would allow testing > the two independently. > Decouple as you wish, but they are both required, and in fact both a high probability of hitting them as I have a build condition (my gitlab ci) which hits both reliably and continuously, and thus neither can pass a build without each other, which is why both additions are in this patch Interestingly on my automated builds, the tests get built before the library which is the requirement for the libcamera_api addition, and the pipeline handlers otherwise get built before the headers. -- Kieran > > Fixes: f870591a9bf5 ("libcamera: properties: Add location property") > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > include/libcamera/meson.build | 2 ++ > > src/libcamera/meson.build | 1 + > > 2 files changed, 3 insertions(+) > > > > diff --git a/include/libcamera/meson.build > b/include/libcamera/meson.build > > index f47c583cbbc0..88edf620f69e 100644 > > --- a/include/libcamera/meson.build > > +++ b/include/libcamera/meson.build > > @@ -44,6 +44,8 @@ foreach header : control_source_files > > install_dir : > join_paths('include', include_dir)) > > endforeach > > > > +libcamera_api += control_headers > > + > > gen_header = files('gen-header.sh') > > > > libcamera_h = custom_target('gen-header', > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > index 88658ac563f7..cd95fa11534a 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -97,6 +97,7 @@ libcamera_deps = [ > > cc.find_library('dl'), > > libudev, > > dependency('threads'), > > + declare_dependency(sources : [control_headers]) > > ] > > > > libcamera_link_with = [] > > -- > Regards, > > Laurent Pinchart >
Hi Kieran, On Sat, Mar 07, 2020 at 04:06:57PM +0000, Kieran Bingham wrote: > On Fri, 6 Mar 2020, 20:35 Laurent Pinchart wrote: > > On Fri, Mar 06, 2020 at 05:20:30PM +0000, Kieran Bingham wrote: > > > The control headers are generated automatically by parsing our YAML > > > descriptions, and creating the control headers. > > > > > > The headers for the controls can be used directly from within libcamera > > > internal pipeline handlers and core components, but there is no link to > > > ensure that the headers are generated before they are used. > > > > > > As part of updating controls to support properties, the commit > > > f870591a9bf5 ("libcamera: properties: Add location property") also > > > inadvertently moved the generated headers out of the libcamera_api > > > dependency generation. > > > > > > This allowed a race condition to occur in builds where objects are > > > attempted to be built before the API definitions had been generated. > > > > > > Declare a dependency on the headers for libcamera to ensure that they > > > are built before compiling any object within the libcamera library, and > > > re-introduce the headers to the libcamera_api variable. > > > > This is bundling two changes in one patch, and given the very low > > probability of hitting the race, and unsuccessful past attempts to fix > > it, I would prefer decoupling the two. > > > > Would you be fine with the first patch carrying the libcamera_api change > > only, with the Fixes: line, and the above paragraph removed, and a > > second patch that adds the declary_dependency ? It would allow testing > > the two independently. > > Decouple as you wish, but they are both required, and in fact both a high > probability of hitting them as I have a build condition (my gitlab ci) > which hits both reliably and continuously, and thus neither can pass a > build without each other, which is why both additions are in this patch I've sent a v3 that decouples them and fixes a few additional issues. > Interestingly on my automated builds, the tests get built before the > library which is the requirement for the libcamera_api addition, and the > pipeline handlers otherwise get built before the headers. There's something that still bothers me slightly, please see below. > > > Fixes: f870591a9bf5 ("libcamera: properties: Add location property") > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > include/libcamera/meson.build | 2 ++ > > > src/libcamera/meson.build | 1 + > > > 2 files changed, 3 insertions(+) > > > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > > > index f47c583cbbc0..88edf620f69e 100644 > > > --- a/include/libcamera/meson.build > > > +++ b/include/libcamera/meson.build > > > @@ -44,6 +44,8 @@ foreach header : control_source_files > > > install_dir : join_paths('include', include_dir)) > > > endforeach > > > > > > +libcamera_api += control_headers > > > + > > > gen_header = files('gen-header.sh') > > > > > > libcamera_h = custom_target('gen-header', > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > index 88658ac563f7..cd95fa11534a 100644 > > > --- a/src/libcamera/meson.build > > > +++ b/src/libcamera/meson.build > > > @@ -97,6 +97,7 @@ libcamera_deps = [ > > > cc.find_library('dl'), > > > libudev, > > > dependency('threads'), > > > + declare_dependency(sources : [control_headers]) The documentation of declare_dependency() specifies the sources argument as follows. "sources, sources to add to targets (or generated header files that should be built before sources including them are built)" This seems correct, but wouldn't it then be simpler to just add control_headers to libcamera_sources ? > > > ] > > > > > > libcamera_link_with = []
diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build index f47c583cbbc0..88edf620f69e 100644 --- a/include/libcamera/meson.build +++ b/include/libcamera/meson.build @@ -44,6 +44,8 @@ foreach header : control_source_files install_dir : join_paths('include', include_dir)) endforeach +libcamera_api += control_headers + gen_header = files('gen-header.sh') libcamera_h = custom_target('gen-header', diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index 88658ac563f7..cd95fa11534a 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -97,6 +97,7 @@ libcamera_deps = [ cc.find_library('dl'), libudev, dependency('threads'), + declare_dependency(sources : [control_headers]) ] libcamera_link_with = []
The control headers are generated automatically by parsing our YAML descriptions, and creating the control headers. The headers for the controls can be used directly from within libcamera internal pipeline handlers and core components, but there is no link to ensure that the headers are generated before they are used. As part of updating controls to support properties, the commit f870591a9bf5 ("libcamera: properties: Add location property") also inadvertently moved the generated headers out of the libcamera_api dependency generation. This allowed a race condition to occur in builds where objects are attempted to be built before the API definitions had been generated. Declare a dependency on the headers for libcamera to ensure that they are built before compiling any object within the libcamera library, and re-introduce the headers to the libcamera_api variable. Fixes: f870591a9bf5 ("libcamera: properties: Add location property") Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- include/libcamera/meson.build | 2 ++ src/libcamera/meson.build | 1 + 2 files changed, 3 insertions(+)