TDE
/
tde-cmake
6
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity
Labels Milestones

Update tde_add_project_* macros #72

Merged
SlavekB merged 2 commits from rfc/docs_translations_macros into master 3 years ago
Conversation 4 Commits 2 Files Changed 1
SlavekB commented 3 years ago
Owner

For the use of macros tde_add_project_* from the top directory of the project I considered immediately when the macros are prepared. @Ray-V, you had a great idea to add a variant with tde_conditional_*!

I modified your proposal so that it supports all variants of calls – with even without using tde_conditional_..., from top directory as well as from a specific subdirectory. This ensures functionality for existing use. To install simple HTML documentation I added potential pictures to the list of installed files. For misc directory I added the removal of Makefile.am file from the list of installed files to allow smooth transition from automake to CMake.

Please, you can test it?

For the use of macros `tde_add_project_*` from the top directory of the project I considered immediately when the macros are prepared. @Ray-V, you had a great idea to add a variant with `tde_conditional_*`! I modified your proposal so that it supports all variants of calls – with even without using `tde_conditional_...`, from top directory as well as from a specific subdirectory. This ensures functionality for existing use. To install simple HTML documentation I added potential pictures to the list of installed files. For `misc` directory I added the removal of `Makefile.am` file from the list of installed files to allow smooth transition from automake to CMake. Please, you can test it?
SlavekB added the PR/rfc label 3 years ago
SlavekB added 2 commits 3 years ago
4910ccc71a
Update tde_add_project_* macros:
65ecb62321
tde_create_handbook: Add *.jpg files to automatically installed.
MicheleC reviewed 3 years ago
modules/TDEMacros.cmake Outdated
LANG ${_lang}
DESTINATION ${_doc_dest}
)
else()
MicheleC commented 3 years ago
Owner

This way the macro either handles docbook OR html files.
Why not making it more flexible and giving the possibility to handle both? Just need to remove the else() and put the html part in a standalone if() block.

This way the macro either handles docbook **OR** html files. Why not making it more flexible and giving the possibility to handle both? Just need to remove the else() and put the html part in a standalone if() block.
SlavekB commented 3 years ago
Poster
Owner

I did it intentionally, because it makes sense to me that either documentation is generated from docbook or manually created in html, but not a combination of both within one language.

I assumed that when we make a change and, for example, add the possibility of creating documentation in AsciiDoc, another else() will be added.

I did it intentionally, because it makes sense to me that either documentation is generated from docbook or manually created in html, but not a combination of both within one language. I assumed that when we make a change and, for example, add the possibility of creating documentation in AsciiDoc, another `else()` will be added.
MicheleC commented 3 years ago
Owner

I understand and I think it is quite sensible. Is there any case where we could have both in the same package? And in such case which one should have priority (docbook or html)?

I understand and I think it is quite sensible. Is there any case where we could have both in the same package? And in such case which one should have priority (docbook or html)?
SlavekB commented 3 years ago
Poster
Owner

If I remember well, there was a case where there was documentation in the docbook, and next to it, some documentation in HTML files that were installed in a different directory – as the code used for misc. Unfortunately I don't remember where it was. But there was no collision that there would be double documentation installed on the same place.

If I remember well, there was a case where there was documentation in the docbook, and next to it, some documentation in HTML files that were installed in a different directory – as the code used for `misc`. Unfortunately I don't remember where it was. But there was no collision that there would be double documentation installed on the same place.
SlavekB commented 3 years ago
Poster
Owner

I found it – TDE/kdiff3

I found it – [TDE/kdiff3](../kdiff3)
MicheleC commented 3 years ago
Owner

ok, thanks Slavek. I guess we can go with the current else() then.

ok, thanks Slavek. I guess we can go with the current else() then.
MicheleC marked this conversation as resolved
MicheleC reviewed 3 years ago
modules/TDEMacros.cmake Outdated
endforeach()
endif()
if( EXISTS ${DOCS_SOURCE_DIR}/misc AND
MicheleC commented 3 years ago
Owner

Uhm.... IMO it would be better to leave the removal of makefile stuff out of TDE macros. The process can be done manually or with a small script and I don't see this as a "programming thing", just a clean up thing.

Uhm.... IMO it would be better to leave the removal of makefile stuff out of TDE macros. The process can be done manually or with a small script and I don't see this as a "programming thing", just a clean up thing.
SlavekB commented 3 years ago
Poster
Owner

You keep in mind that after making a CMake conversion, there is always a period of concurrence with automake before automake related files are deleted. Used list( REMOVE_ITEM ... ) is precisely that the Makefile.am file is not installed in the binary package when CMake build is used.

You keep in mind that after making a CMake conversion, there is always a period of concurrence with automake before automake related files are deleted. Used `list( REMOVE_ITEM ... )` is precisely that the `Makefile.am` file is not installed in the binary package when CMake build is used.
MicheleC commented 3 years ago
Owner

I am fine if you prefer this way, just is seems a bit unclean to me. But I don't have objections to proceed like this.

I am fine if you prefer this way, just is seems a bit unclean to me. But I don't have objections to proceed like this.
SlavekB commented 3 years ago
Poster
Owner

It seems that the misc installation code could be simplified. Instead of file( GLOB ... ) and install( FILES ...), install( DIRECTORY ... ) could be used, where it is possible to use PATERN ... EXLUDE to skip unwanted files.

It seems that the `misc` installation code could be simplified. Instead of `file( GLOB ... )` and `install( FILES ...)`, `install( DIRECTORY ... )` could be used, where it is possible to use `PATERN ... EXLUDE` to skip unwanted files.
SlavekB commented 3 years ago
Poster
Owner

Adjusted as mentioned.

Adjusted as mentioned.
MicheleC commented 3 years ago
Owner

It seems that the misc installation code could be simplified. Instead of file( GLOB ... ) and install( FILES ...), install( DIRECTORY ... ) could be used, where it is possible to use PATERN ... EXLUDE to skip unwanted files.

Looks better. Question: is there any other case other than "misc" folder, that we should consider?

> It seems that the `misc` installation code could be simplified. Instead of `file( GLOB ... )` and `install( FILES ...)`, `install( DIRECTORY ... )` could be used, where it is possible to use `PATERN ... EXLUDE` to skip unwanted files. Looks better. Question: is there any other case other than "misc" folder, that we should consider?
SlavekB commented 3 years ago
Poster
Owner

I considered an other directory. You can notice that in TDE/kvpnc there is a directory misc, which was not currently installed. After adding the default behavior for misc directory, it will now be installed. Therefore, I considered other, which would be without default behavior.

I considered an `other` directory. You can notice that in [TDE/kvpnc](../kvpnc) there is a directory `misc`, which was not currently installed. After adding the default behavior for `misc` directory, it will now be installed. Therefore, I considered `other`, which would be without default behavior.
SlavekB commented 3 years ago
Poster
Owner

Adjusted as mentioned.

Adjusted as mentioned.
MicheleC commented 3 years ago
Owner

Sounds good 👍

Sounds good :+1:
MicheleC marked this conversation as resolved
MicheleC commented 3 years ago
Owner

I only added comments without going as far as requesting changes, since I am just proposing ideas. Overall it looks good though! 👍

I only added comments without going as far as requesting changes, since I am just proposing ideas. Overall it looks good though! :+1:
SlavekB force-pushed rfc/docs_translations_macros from 65ecb62321 to d126204f66 3 years ago
SlavekB force-pushed rfc/docs_translations_macros from d126204f66 to cbba8083f1 3 years ago
Ray-V commented 3 years ago
Collaborator

Comments based on original commits, I can't keep up with you two.

Yes - that works.

Tested with kdbg and kvpnc in the rfc/docs_translations_macro branches.

Tried BUILD_DOC and BUILD_TRANSLATIONS set on and off.

For kdbg, added po directory including Makefile.am, and added Makefile.am back in to translations and doc trees.

Ran the tde-l10n_split_desktop script to set up translations/desktop_files/kdbg.desktop files which created a kdbg.desktop with just the locales selected for the build.

To install simple HTML documentation I added potential pictures to the list of installed files

If there are other types of images/extensions, eg *.svg, *.jpeg, this list will have to be extended.
Wouldn't it be better to GLOB everything in the directory, using the Makefile.am exclusion? Any other files which aren't relevant shouldn't be there.

Similarly for commit 65ecb62321 - tde_create_handbook() is only called once it's been established that *.docbook exists in the directory, so all files there should be relevant to the help files.

Used list( REMOVE_ITEM ... ) is precisely that the Makefile.am file is not installed in the binary package when CMake build is used.

How about removing all Makefile.am during the cmake build?
Add something like this to the top level CMakeLists.txt, and then remove it when the autotools stuff is removed?

file( GLOB_RECURSE _MKFam_files RELATIVE ${CMAKE_SOURCE_DIR} Makefile.am)
foreach( _MKFam_file IN LISTS _MKFam_files )
file( REMOVE ${CMAKE_SOURCE_DIR}/${_MKFam_file} )
endforeach()

It needs to be placed before cmake_install.cmake is created so I put it after set( VERSION R14.1.0 )
I tried file(REMOVE_RECURSE Makefile.am), but that didn't do what I expected.

after making a CMake conversion, there is always a period of concurrence with automake before automake related files are deleted

What's the purpose of this overlap period?

Comments based on original commits, I can't keep up with you two. --- Yes - that works. Tested with kdbg and kvpnc in the rfc/docs_translations_macro branches. Tried BUILD_DOC and BUILD_TRANSLATIONS set on and off. For kdbg, added po directory including Makefile.am, and added Makefile.am back in to translations and doc trees. Ran the tde-l10n_split_desktop script to set up translations/desktop_files/kdbg.desktop files which created a kdbg.desktop with just the locales selected for the build. >To install simple HTML documentation I added potential pictures to the list of installed files If there are other types of images/extensions, eg *.svg, *.jpeg, this list will have to be extended. Wouldn't it be better to GLOB everything in the directory, using the Makefile.am exclusion? Any other files which aren't relevant shouldn't be there. Similarly for commit 65ecb62321 - tde_create_handbook() is only called once it's been established that *.docbook exists in the directory, so all files there should be relevant to the help files. > Used list( REMOVE_ITEM ... ) is precisely that the Makefile.am file is not installed in the binary package when CMake build is used. How about removing all Makefile.am during the cmake build? Add something like this to the top level CMakeLists.txt, and then remove it when the autotools stuff is removed? ``` file( GLOB_RECURSE _MKFam_files RELATIVE ${CMAKE_SOURCE_DIR} Makefile.am) foreach( _MKFam_file IN LISTS _MKFam_files ) file( REMOVE ${CMAKE_SOURCE_DIR}/${_MKFam_file} ) endforeach() ``` It needs to be placed before cmake_install.cmake is created so I put it after *set( VERSION R14.1.0 )* I tried file(REMOVE_RECURSE Makefile.am), but that didn't do what I expected. --- >after making a CMake conversion, there is always a period of concurrence with automake before automake related files are deleted What's the purpose of this overlap period?
SlavekB commented 3 years ago
Poster
Owner

Comments based on original commits, I can't keep up with you two.

Yes - that works.

Tested with kdbg and kvpnc in the rfc/docs_translations_macro branches.

Tried BUILD_DOC and BUILD_TRANSLATIONS set on and off.

For kdbg, added po directory including Makefile.am, and added Makefile.am back in to translations and doc trees.

Ran the tde-l10n_split_desktop script to set up translations/desktop_files/kdbg.desktop files which created a kdbg.desktop with just the locales selected for the build.

To install simple HTML documentation I added potential pictures to the list of installed files

If there are other types of images/extensions, eg *.svg, *.jpeg, this list will have to be extended.
Wouldn't it be better to GLOB everything in the directory, using the Makefile.am exclusion? Any other files which aren't relevant shouldn't be there.

Similarly for commit 65ecb62321 - tde_create_handbook() is only called once it's been established that *.docbook exists in the directory, so all files there should be relevant to the help files.

In directories there sometimes are to find files that are only working and not desirable to be installed. For example, svg files are not used in docbook documentation, but can serve as a png images source. Therefore, it looks like it makes sense that instead of installing everything, only files where it is desirable are installed.

Another reason is that file( GLOB * ) also returned directories, so then the list would have to be processed and the directories deleted from the list. (Option LIST_DIRECTORIES requires CMake 3.3, which is currently undesirable requirement.)

Used list( REMOVE_ITEM ... ) is precisely that the Makefile.am file is not installed in the binary package when CMake build is used.

How about removing all Makefile.am during the cmake build?
Add something like this to the top level CMakeLists.txt, and then remove it when the autotools stuff is removed?

file( GLOB_RECURSE _MKFam_files RELATIVE ${CMAKE_SOURCE_DIR} Makefile.am)
foreach( _MKFam_file IN LISTS _MKFam_files )
file( REMOVE ${CMAKE_SOURCE_DIR}/${_MKFam_file} )
endforeach()

It needs to be placed before cmake_install.cmake is created so I put it after set( VERSION R14.1.0 )
I tried file(REMOVE_RECURSE Makefile.am), but that didn't do what I expected.

No, it's not a good idea to make changes in the source directory. In addition to the Makefile.am files that will be deleted soon there may be files that are required. For example, an apidox generation still depends on the Makefile.am files.

after making a CMake conversion, there is always a period of concurrence with automake before automake related files are deleted

What's the purpose of this overlap period?

Because package maintainers can respond to automake => CMake conversion with delay, here is a rule that automake build can be removed up to one release cycle after CMake conversion. This means that automake rules can now be removed in modules, where the CMake conversion was performed during R14.0.10 cycle.

> Comments based on original commits, I can't keep up with you two. > > --- > > Yes - that works. > > Tested with kdbg and kvpnc in the rfc/docs_translations_macro branches. > > Tried BUILD_DOC and BUILD_TRANSLATIONS set on and off. > > For kdbg, added po directory including Makefile.am, and added Makefile.am back in to translations and doc trees. > > Ran the tde-l10n_split_desktop script to set up translations/desktop_files/kdbg.desktop files which created a kdbg.desktop with just the locales selected for the build. > > >To install simple HTML documentation I added potential pictures to the list of installed files > > If there are other types of images/extensions, eg *.svg, *.jpeg, this list will have to be extended. > Wouldn't it be better to GLOB everything in the directory, using the Makefile.am exclusion? Any other files which aren't relevant shouldn't be there. > > Similarly for commit 65ecb62321 - tde_create_handbook() is only called once it's been established that *.docbook exists in the directory, so all files there should be relevant to the help files. > In directories there sometimes are to find files that are only working and not desirable to be installed. For example, svg files are not used in docbook documentation, but can serve as a png images source. Therefore, it looks like it makes sense that instead of installing *everything*, only files where it is desirable are installed. Another reason is that `file( GLOB * )` also returned directories, so then the list would have to be processed and the directories deleted from the list. (Option `LIST_DIRECTORIES` requires CMake 3.3, which is currently undesirable requirement.) > > > Used list( REMOVE_ITEM ... ) is precisely that the Makefile.am file is not installed in the binary package when CMake build is used. > > How about removing all Makefile.am during the cmake build? > Add something like this to the top level CMakeLists.txt, and then remove it when the autotools stuff is removed? > ``` > file( GLOB_RECURSE _MKFam_files RELATIVE ${CMAKE_SOURCE_DIR} Makefile.am) > foreach( _MKFam_file IN LISTS _MKFam_files ) > file( REMOVE ${CMAKE_SOURCE_DIR}/${_MKFam_file} ) > endforeach() > ``` > It needs to be placed before cmake_install.cmake is created so I put it after *set( VERSION R14.1.0 )* > I tried file(REMOVE_RECURSE Makefile.am), but that didn't do what I expected. > No, it's not a good idea to make changes in the source directory. In addition to the Makefile.am files that *will be deleted soon* there may be files that are required. For example, an apidox generation still depends on the Makefile.am files. > > --- > > >after making a CMake conversion, there is always a period of concurrence with automake before automake related files are deleted > > What's the purpose of this overlap period? > Because package maintainers can respond to automake => CMake conversion with delay, here is a rule that automake build can be removed up to one release cycle after CMake conversion. This means that automake rules can now be removed in modules, where the CMake conversion was performed during R14.0.10 cycle. >
SlavekB force-pushed rfc/docs_translations_macros from cbba8083f1 to 6d6ad623c4 3 years ago
MicheleC approved these changes 3 years ago
SlavekB removed the PR/rfc label 3 years ago
SlavekB merged commit 6d6ad623c4 into master 3 years ago
SlavekB deleted branch rfc/docs_translations_macros 3 years ago
SlavekB added this to the R14.0.11 release milestone 3 years ago
SlavekB commented 3 years ago
Poster
Owner

Thank you all for good ideas and comments!

Thank you all for good ideas and comments!
👍 1

Reviewers

MicheleC approved these changes 3 years ago
The pull request has been merged as 6d6ad623c4.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
MicheleC
Labels
Apply labels
Clear labels
GE/need-info
General - need additional info from contributor PR/keep-branch
Pull request - do not delete branch after merging PR/not-ok
Pull request - need fixing PR/rfc
Pull request - request for comments PR/update-trans
Pull request - update to translation files needed PR/wip
Pull request - work in progress RS/R14.0.x
Related to R14.0.x series RS/R14.1.x
Related to R14.1.x series SL/critical
Severity level - critical SL/major
Severity level - major SL/minor
Severity level - minor SL/normal
Severity level - normal SL/regression
Severity level - regression from previous version SL/trivial
Severity level - trivial SL/wishlist
Severity level - wishlist request ST/duplicate
Status - duplicate of another issue ST/invalid
Status - invalid report ST/notourproblem
Status - not our problem ST/rejected
Status - rejected ST/wontfix
Status - won't fix ST/worksforme
Status - works for me, unable to reproduce
No Label GE/need-info PR/keep-branch PR/not-ok PR/rfc PR/update-trans PR/wip RS/R14.0.x RS/R14.1.x SL/critical SL/major SL/minor SL/normal SL/regression SL/trivial SL/wishlist ST/duplicate ST/invalid ST/notourproblem ST/rejected ST/wontfix ST/worksforme
Milestone
Set milestone
Clear milestone
No items
No Milestone
R14.0.11 release
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Blocks
#9 Install docs and translations using cmake common rules
TDE/kdbg
#5 Remove CMakeLists.txt in doc/ and translations/
TDE/kvpnc
Reference: TDE/tde-cmake#72
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'rfc/docs_translations_macros'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes