Update tde_add_project_* macros #72

Merged
SlavekB merged 2 commits from rfc/docs_translations_macros into master 11 months 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 11 months ago
SlavekB added 2 commits 11 months ago
4910ccc71a
Update tde_add_project_* macros:
65ecb62321
tde_create_handbook: Add *.jpg files to automatically installed.
MicheleC reviewed 11 months ago
LANG ${_lang}
DESTINATION ${_doc_dest}
)
else()
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.
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.
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)?
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.
Poster
Owner

I found it – TDE/kdiff3

I found it – [TDE/kdiff3](../kdiff3)
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 11 months ago
endforeach()
endif()
if( EXISTS ${DOCS_SOURCE_DIR}/misc AND
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.
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.
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.
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.
Poster
Owner

Adjusted as mentioned.

Adjusted as mentioned.
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?
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.
Poster
Owner

Adjusted as mentioned.

Adjusted as mentioned.
Owner

Sounds good 👍

Sounds good :+1:
MicheleC marked this conversation as resolved
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 11 months ago
SlavekB force-pushed rfc/docs_translations_macros from d126204f66 to cbba8083f1 11 months 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?
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 11 months ago
MicheleC approved these changes 11 months ago
SlavekB removed the
PR/rfc
label 11 months ago
SlavekB merged commit 6d6ad623c4 into master 11 months ago
SlavekB deleted branch rfc/docs_translations_macros 11 months ago
SlavekB added this to the R14.0.11 release milestone 11 months ago
Poster
Owner

Thank you all for good ideas and comments!

Thank you all for good ideas and comments!

Reviewers

MicheleC approved these changes 11 months ago
The pull request has been merged as 6d6ad623c4.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Loading…
There is no content yet.