[Insight-developers] openjpg include dir not exported
Marcus D. Hanwell
marcus.hanwell at kitware.com
Sat Aug 28 18:07:48 EDT 2010
On Sat, Aug 28, 2010 at 3:58 PM, Luis Ibanez <luis.ibanez at kitware.com>wrote:
>
> When we identify a bug during the review, or simply
> something that can be improved in the code,... is it
> possible to make the modification in our local git
> repository, and push it back to Gerrit, and have it
> associated to the code that we were reviewing ?
>
Yes, you could make a dependent commit. So when you copy and paste the line
to fetch the change, you can do a,
git checkout -b topic-name
This will give you a topic branch, and it will start with the commit you are
reviewing. You can then make the necessary changes, and then commit those
locally. You could then upload the dependent commit to Gerrit,
git push gerrit HEAD:refs/for/master/topic-name
Your new commit would appear, and it would depend upon the commit you are
reviewing.
>
> For example, for the comments that I put in
>
> http://review.source.kitware.com/#change,5
>
> It probably would have made more sense to patch
> the patch, and have that modified patch be related
> to the original change in Gerrit.
>
You can also edit the commit. If it has the Change-Id: line in the commit
message, then amending the commit and pushing would show up in Gerrit as an
updated patch (patch set 2, 3 etc).
>
>
> On the other hand,
>
> Looking at the "Life of a Patch" for Android:
> http://source.android.com/source/life-of-a-patch.html
>
> It seems that they always send modifications back
> to the original authors...
>
>
> What would you recommend as a best practice ?
>
> I think it depends on the nature of the change. If it was small I would
make an inline comment in the code, pointing out the issue and asking the
author to update their patch set. If it was bigger I would comment that I
had uploaded a dependent patch set (a new commit) that corrected these
issues.
Gerrit can handle both of these use cases. So long as the Change-Id line is
in the commit message it will figure out that you updated the patch and show
it in the web interface as an updated patch - I am going to update the wiki
page as the new way is much simpler than the one I explained (which works,
but is deprecated).
I saw some posts where you talked about which lines were wrong - in the
side-by-side diff you can double click on the offending line and an editor
will pop up. You can enter your comments and they will be shown as comments
made by you, and the submitter will be emailed. These comments are saved
when you review the commit, and make an overall comment on the patch, assign
a score etc.
Thanks,
Marcus
>
> On Sat, Aug 28, 2010 at 2:23 PM, Marcus D. Hanwell <
> marcus.hanwell at kitware.com> wrote:
>
>> On Sat, Aug 28, 2010 at 2:18 PM, Luis Ibanez <luis.ibanez at kitware.com>wrote:
>>
>>> Hi Brad,
>>>
>>> That's a good point.
>>>
>>> I'm not sure if Gerrit should be aware of conflicts between patches.
>>> After all, they are supposed to be in different branches, isn't it ?
>>>
>>
>> Gerrit does not assess conflicts between review requests, this should be
>> handled by Git when merging the patch in.
>>
>>>
>>> ---
>>>
>>> I'm now testing your patches (well.. figuring out how to do it..)
>>> will merge them with the openjpegpimpl branch before pushing
>>> them.
>>>
>>
>> You can fetch the final commit in the topic branch, using the copy/paste
>> instructions, then git checkout -b tree, give it a topic name. You can then
>> merge that topic branch into yours (move to your topic branch, and then git
>> merge other-topic). At that point you can resolve the conflict (if there is
>> one), and once you are done that topic can be staged and merged.
>>
>> Hope that makes sense.
>>
>> Marcus
>>
>>>
>>> -----------------------------------
>>>
>>> On Sat, Aug 28, 2010 at 2:02 PM, Bradley Lowekamp <
>>> blowekamp at mail.nih.gov> wrote:
>>>
>>>> Luis,
>>>>
>>>> That is a great idea to use a private implementation. Should help with
>>>> compile time too (every little bit helps)!
>>>>
>>>> Unfortunately, your patch is going to conflict with mine. This is
>>>> interesting in Gerrit because I would expect something to come up to showing
>>>> that there is a conflict between these two topic. But I don't see anything.
>>>>
>>>> Luis you started to review my topic. Do you think it's good to go? So I
>>>> can hurry up to stage and merge my topic, before yours :P ( I already had
>>>> the burden of rebasing and merging after the uncrustification )
>>>>
>>>> Brad
>>>>
>>>> On Aug 28, 2010, at 12:17 PM, Luis Ibanez wrote:
>>>>
>>>> BTW,
>>>>
>>>> We probably could use PIMPL in the itkJPEG2000ImageIO
>>>> class, so that the headers and data structures of openjpeg
>>>> are not exposed to ITK developers...
>>>>
>>>> I'll give it a try...
>>>>
>>>>
>>>> Luis
>>>>
>>>>
>>>>
>>>> --------------------------------------------------------------------------
>>>> On Sat, Aug 28, 2010 at 12:15 PM, Luis Ibanez <luis.ibanez at kitware.com>wrote:
>>>>
>>>>>
>>>>> Hi Gaetan,
>>>>>
>>>>>
>>>>> Thanks for the clarification.
>>>>>
>>>>>
>>>>> Can you please try this patch:
>>>>>
>>>>> diff --git a/itkIncludeDirectories.cmake b/itkIncludeDirectories.cmake
>>>>> index 36b2ab5..38584fb 100644
>>>>> --- a/itkIncludeDirectories.cmake
>>>>> +++ b/itkIncludeDirectories.cmake
>>>>> @@ -37,6 +37,7 @@ SET(ITK_INCLUDE_DIRS_BUILD_TREE
>>>>> ${ITK_INCLUDE_DIRS_BUILD_TREE}
>>>>> ${ITK_SOURCE_DIR}/Utilities/nifti/niftilib
>>>>> ${ITK_SOURCE_DIR}/Utilities/nifti/znzlib
>>>>> ${ITK_SOURCE_DIR}/Utilities/itkExtHdrs
>>>>> + ${ITK_SOURCE_DIR}/Utilities/openjpeg
>>>>> ${ITK_BINARY_DIR}/Utilities
>>>>> ${ITK_SOURCE_DIR}/Utilities
>>>>> )
>>>>> @@ -149,6 +150,7 @@ SET(ITK_INCLUDE_RELATIVE_DIRS
>>>>> ${ITK_INCLUDE_RELATIVE_DIRS}
>>>>> Utilities/nifti/niftilib
>>>>> Utilities/nifti/znzlib
>>>>> Utilities/itkExtHdrs
>>>>> + Utilities/openjpeg
>>>>> Utilities
>>>>> )
>>>>>
>>>>>
>>>>> and if it works,
>>>>> please go ahead and push the change.
>>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>> Luis
>>>>>
>>>>>
>>>>> -----------------------------------------------------------
>>>>> 2010/8/28 Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr>
>>>>>
>>>>>
>>>>>> Le 28 août 10 à 00:53, Luis Ibanez a écrit :
>>>>>>
>>>>>>
>>>>>>> Hi Gaetan,
>>>>>>>
>>>>>>
>>>>>> Hi Luis,
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Are you doing this with an installed version of ITK ?
>>>>>>>
>>>>>>
>>>>>> No, I'm using it in the build tree.
>>>>>>
>>>>>>
>>>>>>
>>>>>>> I just verified that I can use JPEG2000ImageIO
>>>>>>> from a small example outside of the ITK build tree.
>>>>>>>
>>>>>>> (Note that you must have configured ITK with
>>>>>>> ITK_USE_REVIEW ON).
>>>>>>>
>>>>>>
>>>>>> Yes, that's on.
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Please let me know how I can reproduce the problem
>>>>>>> that you are observing.
>>>>>>>
>>>>>>
>>>>>> I'm trying to add JPEG2000ImageIO to wrapitk, but openjpeg.h can't be
>>>>>> found.
>>>>>> I have to add the path to this file in ITK_INCLUDE_DIRS in
>>>>>> ITKConfig.cmake
>>>>>>
>>>>>> Without this manual fix, I get the following error message:
>>>>>>
>>>>>> [ 80%] Generating wrap_ITKIOBase.xml
>>>>>> In file included from
>>>>>> /home/glehmann/src/tests/wrapitk/build/Libraries/IO/wrap_ITKIOBase.cxx:77:
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:35:24:
>>>>>> error: openjpeg.h: No such file or directory
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:36:19:
>>>>>> error: j2k.h: No such file or directory
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:37:19:
>>>>>> error: jp2.h: No such file or directory
>>>>>> In file included from
>>>>>> /home/glehmann/src/tests/wrapitk/build/Libraries/IO/wrap_ITKIOBase.cxx:77:
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:111:
>>>>>> error: 'opj_dparameters_t' does not name a type
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:130:
>>>>>> error: ISO C++ forbids declaration of 'opj_codec_t' with no type
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:130:
>>>>>> error: expected ';' before '*' token
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:132:
>>>>>> error: 'OPJ_UINT32' does not name a type
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:133:
>>>>>> error: 'OPJ_UINT32' does not name a type
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:135:
>>>>>> error: 'OPJ_UINT32' does not name a type
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:136:
>>>>>> error: 'OPJ_UINT32' does not name a type
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:138:
>>>>>> error: 'OPJ_UINT32' does not name a type
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:139:
>>>>>> error: 'OPJ_UINT32' does not name a type
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h: In
>>>>>> member function 'void itk::JPEG2000ImageIO::SetTileSize(int, int)':
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:102:
>>>>>> error: 'm_TileWidth' was not declared in this scope
>>>>>> /home/glehmann/src/tests/ITK/Code/Review/itkJPEG2000ImageIO.h:103:
>>>>>> error: 'm_TileHeight' was not declared in this scope
>>>>>> make[3]: *** [Libraries/IO/wrap_ITKIOBase.xml] Erreur 1
>>>>>> make[2]: *** [Libraries/IO/CMakeFiles/IOIdx.dir/all] Erreur 2
>>>>>> make[1]: *** [Libraries/IO/CMakeFiles/IOPython.dir/rule] Erreur 2
>>>>>> make: *** [IOPython] Erreur 2
>>>>>>
>>>>>> Gaëtan
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>>
>>>>>>> Luis
>>>>>>>
>>>>>>>
>>>>>>> -------------------
>>>>>>> 2010/8/27 Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr>
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I found a small problem with openjpg: the include dir is not
>>>>>>> exported, and thus openjpeg.h can't be found while using JPEG2000ImageIO
>>>>>>> outside of the ITK build tree.
>>>>>>> I tried to fix that but I'm not sure how to do it right.
>>>>>>>
>>>>>>> It would be nice if someone used to how the utilities are managed in
>>>>>>> ITK can fix that!
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Gaëtan
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Gaëtan Lehmann
>>>>>>> Biologie du Développement et de la Reproduction
>>>>>>> INRA de Jouy-en-Josas (France)
>>>>>>> tel: +33 1 34 65 29 66 fax: 01 34 65 29 09
>>>>>>> http://voxel.jouy.inra.fr http://www.itk.org
>>>>>>> http://www.mandriva.org http://www.bepo.fr
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Powered by www.kitware.com
>>>>>>>
>>>>>>> Visit other Kitware open-source projects at
>>>>>>> http://www.kitware.com/opensource/opensource.html
>>>>>>>
>>>>>>> Kitware offers ITK Training Courses, for more information visit:
>>>>>>> http://kitware.com/products/protraining.html
>>>>>>>
>>>>>>> Please keep messages on-topic and check the ITK FAQ at:
>>>>>>> http://www.itk.org/Wiki/ITK_FAQ
>>>>>>>
>>>>>>> Follow this link to subscribe/unsubscribe:
>>>>>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> --
>>>>>> Gaëtan Lehmann
>>>>>> Biologie du Développement et de la Reproduction
>>>>>> INRA de Jouy-en-Josas (France)
>>>>>> tel: +33 1 34 65 29 66 fax: 01 34 65 29 09
>>>>>> http://voxel.jouy.inra.fr http://www.itk.org
>>>>>> http://www.mandriva.org http://www.bepo.fr
>>>>>>
>>>>>>
>>>>>
>>>> <ATT00001..txt>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Powered by www.kitware.com
>>>>
>>>> Visit other Kitware open-source projects at
>>>> http://www.kitware.com/opensource/opensource.html
>>>>
>>>> Kitware offers ITK Training Courses, for more information visit:
>>>> http://kitware.com/products/protraining.html
>>>>
>>>> Please keep messages on-topic and check the ITK FAQ at:
>>>> http://www.itk.org/Wiki/ITK_FAQ
>>>>
>>>> Follow this link to subscribe/unsubscribe:
>>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>>
>>>>
>>>
>>> _______________________________________________
>>> Powered by www.kitware.com
>>>
>>> Visit other Kitware open-source projects at
>>> http://www.kitware.com/opensource/opensource.html
>>>
>>> Kitware offers ITK Training Courses, for more information visit:
>>> http://kitware.com/products/protraining.html
>>>
>>> Please keep messages on-topic and check the ITK FAQ at:
>>> http://www.itk.org/Wiki/ITK_FAQ
>>>
>>> Follow this link to subscribe/unsubscribe:
>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20100828/caf58596/attachment.htm>
More information about the Insight-developers
mailing list