MantisBT - CMake
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0012129CMakeCPackpublic2011-04-27 15:122012-02-29 10:17
ReporterDaniel Nelson 
Assigned ToEric NOULARD 
PrioritynormalSeverityfeatureReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product VersionCMake 2.8.4 
Target VersionCMake 2.8.8Fixed in VersionCMake 2.8.8 
Summary0012129: Add top level directory to component install
DescriptionWhen CPACK_ARCHIVE_COMPONENT_INSTALL is on, the packages do not contain a top level directory, unlike with the non component install. This is true even if CPACK_INCLUDE_TOPLEVEL_DIRECTORY is enabled.

It would be nice if a top level directory could be added. This would mirror the behavior of the non component install and prevent an annoying surprise for anyone untarring without listing the contents first.

I also think having this top level directory should be the default, since CPACK_INCLUDE_TOPLEVEL_DIRECTORY=1 is the default for archives.
Steps To Reproduce
Additional Information
TagsNo tags attached.
Relationships
related to 0012606closed Eric NOULARD Archive and STGZ packages don't respect CPACK_PACKAGE_INSTALL_DIRECTORY (patch included) 
related to 0013004closed Eric NOULARD Archive generator (TGZ, ZIP etc) doesn't work if component-based packaging is used with CPACK_SET_DESTDIR set to ON 
Attached Filespatch 0001-Add-toplevel-directory-to-component-install.patch (2,239) 2011-05-01 22:47
https://public.kitware.com/Bug/file/3842/0001-Add-toplevel-directory-to-component-install.patch
Issue History
Date ModifiedUsernameFieldChange
2011-04-27 15:12Daniel NelsonNew Issue
2011-04-27 16:44Eric NOULARDAssigned To => Eric NOULARD
2011-04-27 16:44Eric NOULARDStatusnew => assigned
2011-05-01 22:47Daniel NelsonFile Added: 0001-Add-toplevel-directory-to-component-install.patch
2011-05-01 22:48Daniel NelsonNote Added: 0026296
2011-12-12 17:32Eric NOULARDRelationship addedrelated to 0012606
2012-01-13 16:08Eric NOULARDNote Added: 0028299
2012-01-13 16:09Eric NOULARDTarget Version => CMake 2.8.8
2012-01-22 13:33Eric NOULARDNote Added: 0028389
2012-01-22 13:33Eric NOULARDStatusassigned => resolved
2012-01-22 13:33Eric NOULARDFixed in Version => CMake 2.8.8
2012-01-22 13:33Eric NOULARDResolutionopen => fixed
2012-01-24 14:42David ColeNote Added: 0028402
2012-01-24 15:00Eric NOULARDNote Added: 0028403
2012-01-24 15:00Eric NOULARDStatusresolved => feedback
2012-01-24 15:00Eric NOULARDResolutionfixed => reopened
2012-02-01 15:33David ColeNote Added: 0028437
2012-02-01 15:50Eric NOULARDNote Added: 0028438
2012-02-01 16:24David ColeNote Added: 0028439
2012-02-01 17:06Eric NOULARDNote Added: 0028440
2012-02-01 17:42Eric NOULARDNote Added: 0028441
2012-02-25 17:51Eric NOULARDNote Added: 0028723
2012-02-25 17:51Eric NOULARDStatusfeedback => closed
2012-02-25 17:51Eric NOULARDResolutionreopened => fixed
2012-02-29 10:17Eric NOULARDRelationship addedrelated to 0013004

Notes
(0026296)
Daniel Nelson   
2011-05-01 22:48   
The patch creates a top level directory under each component directory if
CPACK_INCLUDE_TOPLEVEL_DIRECTORY is set (which is on by default). So you end
up with paths in the achive like:
 foo-1.0.0-Linux/bin/foo

And paths in the build directory like:
 _CPack_Packages/Linux/TGZ/foo-1.0.0-Linux/component_group/foo-1.0.0-Linux/bin/foo

This is a little repetitive, ideally the path would be more like:
 _CPack_Packages/Linux/TGZ/component_group/foo-1.0.0-Linux/bin/foo

However, this would be a bit more evasive because I think we would want to remove CPACK_TEMPORARY_DIRECTORY and related variables, and compute this directory later in InstallProjectViaInstallCMakeProjects when we know if we are doing a component install or not.
(0028299)
Eric NOULARD   
2012-01-13 16:08   
Hi Daniel,

Just reviewing your patch.
I don't think the repetition in the path is a big deal and in fact
it may be fine for archive generators but
may be annoying for others generators, thus I don't think it's a big
deal to keep it this way.

Your patch is ok.
I just pushed the corresponding branch on stage/AddTopLevelForComponent.

I did not merge it to next yet because I'd like to have some wider
feedback before changing a default behavior.

Before this componentized archive installers (ZIP, TGZ, STGZ etc...) wouldn't
have this toplevel after CMake 2.8.8 or whatever version this is merged in
then they will get that. This may break backward compatibility.

The older behavior may be restored by setting CPACK_INCLUDE_TOPLEVEL_DIRECTORY
to 0 which is fine but I'd like to poll about this.
(0028389)
Eric NOULARD   
2012-01-22 13:33   
No one screamed for more than a week, let's go for this.

Merge topic 'AddTopLevelForComponent' into next
3cab24a CPack Add top level directory in component install for Archive Generators
(0028402)
David Cole   
2012-01-24 14:42   
So, what about backwards compatibility? What if people are depending on the existing layout? We are not going to merge this to master until we answer that question...
(0028403)
Eric NOULARD   
2012-01-24 15:00   
Hi Dave,

Ok re-opening this.

I think I do care as much you do about backward compat'.
That's why I asked for comment on the ML:
http://www.cmake.org/pipermail/cmake/2012-January/048594.html [^]
then deciced to go forward (a little less than 10 days later)
because I did not get any feedback:
http://www.cmake.org/pipermail/cmake/2012-January/048752.html [^]

So 2 possibilities:
  1) People don't care about this because no-one relies on this.
  2) People don't take time to test and will only scream
     when this goes to master or may be first RC

What would you suggest I do when I did not get any feedback?

My personal opinion is that almost no-one is relying
on the current behavior because if they did I'm pretty sure
they would have screamed.
My second thought is that the current behavior is inconsistent
and may be considered as a bug that's why I decided to change
the default behavior.

All that said I may simply test if we are doing
component packaging and have a default behavior which is
different in the monolithic case than in the component
case. Technically this is easy, but I still find it inconsistent.

What do you suggest?
(0028437)
David Cole   
2012-02-01 15:33   
Will this change make anybody mad, or cause anybody to spend hours trying to figure out what changed underneath them and how to get back to the old behavior?

If you are 99.9% convinced that it is better to make the default behavior change, then I will entertain making the change...

You say it's easy to go back to the old behavior "by setting CPACK_INCLUDE_TOPLEVEL_DIRECTORY to 0" -- if that's true, at least people who do encounter the behavior change can have a way to get back to the old behavior.

Even though we've not seen anybody screaming on the mailing list about this, it still makes me nervous to change the default behavior of CPack intentionally.
(0028438)
Eric NOULARD   
2012-02-01 15:50   
Hi Dave,

> Will this change make anybody mad, or cause anybody to spend hours trying to
> figure out what changed underneath them and how to get back to the old behavior?

Difficult to tell.
It's not like if the archive wasn't valid, if it happen they'll discover it
when un-archiving...

> If you are 99.9% convinced that it is better to make the default behavior
> change, then I will entertain making the change...

Yes I am, but it leaves 0.01 of error :-]

I do because I think the initial behavior was an error, there is
no reason to have a default behavior different in the component case.

> Even though we've not seen anybody screaming on the mailing list about this,
> it still makes me nervous to change the default behavior of CPack
> intentionally.

Agreed, it is difficult to evaluate how many people already use this.
I think it should be relatively low because the default behavior
of ArchiveGenerator is to be MONOLITHIC (backward compat' choice)
not componentized.

The safest path would be to keep the default behavior as before
and requiring user to
set(CPACK_INCLUDE_TOPLEVEL_DIRECTORY 1)
if they want the new more consistent behavior...

It is doable if we check whether if CPACK_INCLUDE_TOPLEVEL_DIRECTORY was
set by the user prior to the
this->SetOptionIfNotSet("CPACK_INCLUDE_TOPLEVEL_DIRECTORY", "1");
in
 cmCPackArchiveGenerator::InitializeInternal()
(0028439)
David Cole   
2012-02-01 16:24   
What we know:

- the reporter of the bug has experienced the bug and wants a way to change the behavior (not necessarily the default behavior, but just be able to change it)

What I strongly suspect:

- there is also somebody out there perfectly content with the existing behavior who will not understand why his package is now laid out differently even though he made no changes besides updating to the latest CMake/CPack.

I would like to see the default behavior remain as is, even if it's "not self-consistent across all CPack generators"

Please either add a new variable to control this instead of CPACK_INCLUDE_TOPLEVEL_DIRECTORY, or figure out some other way to preserve the existing behavior, but give the reporter of this issue a way to achieve his goal.
(0028440)
Eric NOULARD   
2012-02-01 17:06   
Ok right.
I'll do that.
(0028441)
Eric NOULARD   
2012-02-01 17:42   
I have just implemented the desired backward compatible behavior.
user shall
set(CPACK_COMPONENT_INCLUDE_TOPLEVEL_DIRECTORY ON)
if he wants to get the "toplevel included" behavior in the component case.
The default being "as before" not to include the toplevel dir in the
component case.

This should be fully backward compatible.

I did push forward the branch and merged it to next:

Merge topic 'AddTopLevelForComponent' into next

5d18851 CPackArchive restore default behavior and provide new variable.
(0028723)
Eric NOULARD   
2012-02-25 17:51   
This has been merged to master.