Bug 97419 - [CMake] Change hardcoded 'lib' and 'bin' in CMakeLists.txt to 'LIB_INSTALL_DIR' and 'EXEC_INSTALL_DIR'
Summary: [CMake] Change hardcoded 'lib' and 'bin' in CMakeLists.txt to 'LIB_INSTALL_DI...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Byungwoo Lee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-23 19:06 PDT by Byungwoo Lee
Modified: 2012-09-24 20:21 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.40 KB, patch)
2012-09-24 02:38 PDT, Byungwoo Lee
no flags Details | Formatted Diff | Diff
Patch (2.30 KB, patch)
2012-09-24 08:57 PDT, Byungwoo Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Byungwoo Lee 2012-09-23 19:06:03 PDT
There is hardcoded 'lib' and 'bin' in CMakeLists.txt,
and this might be better to be changed to 'LIB_INSTALL_DIR' and 'EXEC_INSTALL_DIR'.
Comment 1 Byungwoo Lee 2012-09-23 21:26:19 PDT
I tested on Debug/Release and it works ok.
Comment 2 Byungwoo Lee 2012-09-24 02:38:41 PDT
Created attachment 165337 [details]
Patch
Comment 3 Patrick R. Gansterer 2012-09-24 08:35:13 PDT
Comment on attachment 165337 [details]
Patch

look ok to me
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-09-24 08:36:13 PDT
Comment on attachment 165337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165337&action=review

> ChangeLog:3
> +        Change hardcoded 'lib' and 'bin' in CMakeLists.txt to 'LIB_INSTALL_DIR' and 'EXEC_INSTALL_DIR'.

Please use the [CMake] prefix for CMake-related bugs.

> ChangeLog:11
> +        Currently, 'LIB_INSTALL_DIR' and 'EXEC_INSTALL_DIR' is used for
> +        hardcoded 'lib' and 'bin' directory.
> +        Apply this to set the below variables.
> +        CMAKE_ARCHIVE_OUTPUT_DIRECTORY, CMAKE_LIBRARY_OUTPUT_DIRECTORY, CMAKE_RUNTIME_OUTPUT_DIRECTORY

This explanation is hard to understand. Just saying something like "Use the value of LIB_INSTALL_DIR and EXEC_INSTALL_DIR instead of hardcoding lib and bin for CMAKE_{ARCHIVE,LIBRARY,RUNTIME}_OUTPUT_DIRECTORY" would be easier to grasp.
Comment 5 Byungwoo Lee 2012-09-24 08:47:24 PDT
Comment on attachment 165337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165337&action=review

>> ChangeLog:3
>> +        Change hardcoded 'lib' and 'bin' in CMakeLists.txt to 'LIB_INSTALL_DIR' and 'EXEC_INSTALL_DIR'.
> 
> Please use the [CMake] prefix for CMake-related bugs.

Ok~ I'll add it.

>> ChangeLog:11
>> +        CMAKE_ARCHIVE_OUTPUT_DIRECTORY, CMAKE_LIBRARY_OUTPUT_DIRECTORY, CMAKE_RUNTIME_OUTPUT_DIRECTORY
> 
> This explanation is hard to understand. Just saying something like "Use the value of LIB_INSTALL_DIR and EXEC_INSTALL_DIR instead of hardcoding lib and bin for CMAKE_{ARCHIVE,LIBRARY,RUNTIME}_OUTPUT_DIRECTORY" would be easier to grasp.

Ok~ it'll be more clear :)
Comment 6 Ming Xie 2012-09-24 08:53:07 PDT
Sorry, why do you want to remove this line?

-INCLUDE(OptionsCommon)
Comment 7 Byungwoo Lee 2012-09-24 08:57:30 PDT
Created attachment 165393 [details]
Patch
Comment 8 Byungwoo Lee 2012-09-24 09:23:36 PDT
(In reply to comment #6)
> Sorry, why do you want to remove this line?
> 
> -INCLUDE(OptionsCommon)
The line is not removed. it is moved to the above (#132).
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-09-24 10:10:55 PDT
Comment on attachment 165393 [details]
Patch

Looks fine, thanks.
Comment 10 Ming Xie 2012-09-24 10:28:50 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > Sorry, why do you want to remove this line?
> > 
> > -INCLUDE(OptionsCommon)
> The line is not removed. it is moved to the above (#132).

I don't know how I missed that. Sorry! :(
Comment 11 Byungwoo Lee 2012-09-24 18:31:18 PDT
(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #6)
> > > Sorry, why do you want to remove this line?
> > > 
> > > -INCLUDE(OptionsCommon)
> > The line is not removed. it is moved to the above (#132).
> 
> I don't know how I missed that. Sorry! :(

No problem :)
Comment 12 WebKit Review Bot 2012-09-24 20:21:54 PDT
Comment on attachment 165393 [details]
Patch

Clearing flags on attachment: 165393

Committed r129447: <http://trac.webkit.org/changeset/129447>
Comment 13 WebKit Review Bot 2012-09-24 20:21:58 PDT
All reviewed patches have been landed.  Closing bug.