Community
Participate
Working Groups
Currently, users pretty much always have to have the .project and .cproject files either checked out or hijacked when they're working with source-control. The .project and .cproject files are written to even if there are no structural changes. Here are a few scenarios: 1. When changing the currently active build-configuration on a project, both the .project and the .cproject files are touched unnecessarily. The .cproject file doesn't change structurally (although a couple of XML attributes swap places sometimes). The .project file does change, but only because it stores too much unnecessary information for the managed-builder - the builder-arguments are stored even if user leaves them at their default values. 2. Also, related to the above scenario - the active build-configuration itself is never persisted anywhere, CDT just takes the first configuration as the active one when an existing project is imported. So if user changes the active configuration to the second one, and then deletes/reimports the project, the .project file is touched to synchronize the build-location argument with the currently active build-configuration. Again, the file gets modified because it stores too much unnecessary information. 3. Opening the "C/C++ Build > Settings" property page and clicking OK without touching anything also rewrites the .cproject file with identical contents.
This is somewhat related to bug 331031 and bug 226457. The .project file should stop being touched when we move to using platform build configurations. The .cproject on the other hand is harder, and caused many users pain (bug 226457). Any patch contributions you could make would be appreciated.
Created attachment 207943 [details] Proposed patch to address .project file modifications Attaching a proposed patch to fix unnecessary modifications of the .project file (addressing only half of the problem reported). As I see it, the main problem is that CDT builder-settings, which are persisted in the .cproject model, are also duplicated, for the active configuration, in the Platform build-command (persisted in the .project file). When the active configuration is changed, the .project file is always rewritten to reflect the newly active configuration's builder-settings. As I see it, there appears to be no need to duplicate these builder-settings in the .project file because these do not appear to be used anywhere. To be more precise - the builder-settings persisted into the .project file are controlled by BuilderFactory.CONTENTS_ACTIVE_CFG_SETTINGS. This constant is only referenced twice - once in BuilderFactory.builderBuildArgsMap(..), which produces the content; and once in BuilderFactory.createBuilders(..), which is meant to consume the content. But, BuilderFactory.createBuilders(..) doesn't actually consume any of the produced content aside from the content-type itself (which is BuilderFactory.CONTENTS_ACTIVE_CFG_SETTINGS). Instead, it just obtains the builder-settings directly out of the CDT builder-model. So, my proposal is to cut this unnecessary persistence of builder-settings into the build-command, but only persist the content-type, based on which the builder-settings are obtained directly from the CDT model.
Created attachment 209466 [details] Proposed patch to address .cproject file modifications Attaching a proposed patch to fix unnecessary modifications of the .cproject file (addressing the remaining half of the problem reported). The patch addresses the problem where changing the active build-configuration causes unnecessary re-serialization of the project-description (into the .cproject file). The re-serialization is unnecessary because the active build-configuration isn't even stored in the project-description - it's stored as a persistent property of the project resource (in the workspace metadata). Same applies to the setting build-configuration (used for indexing). The reason that changing the active build-configuration triggers re-serialization of the project-description is because the CProjectDescription.isModified() method must include the active/setting build-configuration dirty-state in order to properly trigger all necessary events which are expected when the active build-configuration changes. The only undesirable side-effect is that the project-description is unnecessarily re-serialized. So, to avoid unnecessary re-serialization of the project-description, I've added a new method, CProjectDescription.needsDescriptionPersistence(). Logically, it's equivalent to the isModified() method minus the active/setting build-configuration dirty-state. This new method is only used as the condition when serializing the project-description - it will no longer happen if the only thing that's changed is the active/setting build-configuration choice. The isModified() method is logically unchanged, so the rest of the behaviour remains unchanged.
Created attachment 209991 [details] Testcase to illustrate the bug, and regression-test the fixes Attaching a patch containing the testcase that can be used to regression-test the two fixes. The same testcase also illustrates the bug described (if run before applying the two fix patches).
The fact that active status of configuration is not persisted in .cproject actually causes problems, there are a couple of bugs about that, see bug 278771 for one. Your change related to active configuration is at odds with those requests. Should the active status be saved in project metadata? I haven't developed a strong opinion about it yet. What is your take on that?
Thanks for looking into this, Andrew. My take on this is that looking at older CDT versions (CDT v3.x), the active-configuration setting was always a user preference, so it was always being lost during project re-import. At the same time, the .cdtbuild/.cdtproject files were never touched when the CDT v3.x user changed his active-configuration. So, based on that, I view bug 278771 as a request for new behaviour, while this bug (and bug 226457) are regression bugs. It definitely would be nice to have a design which allows more flexibility to let the user (or the CDT-based product vendor) to chose whether user-preferences are stored in the project or not. Or to chose what is considered to be a user-preference and what is not. I would vote for continuing the bug 226457 discussion for the long term solution, but fix the regression for the short term.
(In reply to comment #6) > Thanks for looking into this, Andrew. > My take on this is that looking at older CDT versions (CDT v3.x), the > active-configuration setting was always a user preference, so it was always > being lost during project re-import. At the same time, the > .cdtbuild/.cdtproject files were never touched when the CDT v3.x user changed > his active-configuration. So, based on that, I view bug 278771 as a request for > new behaviour, while this bug (and bug 226457) are regression bugs. Storing the selection of the active build configuration in .project, .cproject, or something under .settings, is - and will be - causing constant pain because of all the merge conflicts just by having different developers switching back and forth between debug and release. I tend to see the active build configuration as kind of the target platform in PDE, and as such is something that is stored in your workspace (however associated to your project, of course). This would also resolve the contradiction between 278771 (yes, my selection should be persistent between different imports into the same workspace) and 226457 (no, switching between build configurations should not cause merge conflicts in a revision control system). So, I really welcome Baltasar's approach to the first part of the solution.
(In reply to comment #2) > Created attachment 207943 [details] > Proposed patch to address .project file modifications > Attaching a proposed patch to fix unnecessary modifications of the .project file > (addressing only half of the problem reported). > As I see it, the main problem is that CDT builder-settings, which are persisted > in the .cproject model, are also duplicated, for the active configuration, in > the Platform build-command (persisted in the .project file). When the active > configuration is changed, the .project file is always rewritten to reflect the > newly active configuration's builder-settings. As I see it, there appears to be > no need to duplicate these builder-settings in the .project file because these > do not appear to be used anywhere. > > To be more precise - the builder-settings persisted into the .project file are > controlled by BuilderFactory.CONTENTS_ACTIVE_CFG_SETTINGS. This constant is only > referenced twice - once in BuilderFactory.builderBuildArgsMap(..), which > produces the content; and once in BuilderFactory.createBuilders(..), which is > meant to consume the content. But, BuilderFactory.createBuilders(..) doesn't > actually consume any of the produced content aside from the content-type itself > (which is BuilderFactory.CONTENTS_ACTIVE_CFG_SETTINGS). Instead, it just obtains > the builder-settings directly out of the CDT builder-model. Could you elaborate how you arrived to this conclusion? As far as I see, USE_DEFAULT_BUILD_CMD is stored in the map independently from CONTENTS_ACTIVE_CFG_SETTINGS. I see USE_DEFAULT_BUILD_CMD - which you removed - is being used to create custom builders in createBuilder() but I do not see CONTENTS_ACTIVE_CFG_SETTINGS around. > > So, my proposal is to cut this unnecessary persistence of builder-settings into > the build-command, but only persist the content-type, based on which the > builder-settings are obtained directly from the CDT model.
The createBuilder() private method is called from two places - createBuilders() and createBuilderFromCommand(). The createBuilders() route is taken care of by the CONTENTS_ACTIVE_CFG_SETTINGS switch. But the createBuilderFromCommand() route is very obscure, I agree. The way I see it, the createBuilderFromCommand() method first clones the configuration's Builder object [createCustomBuilder() on line 313], then it forces all the BuildCommand arguments onto the new Builder instance [builder.loadFromProject() on line 333]. In the particular use-case, which is affected by my patch, the BuildCommand will simply contain the flattened set of all the Builder arguments (obtained by recursively collecting all the superClass arguments, overlaying them with instance arguments). So, in this use-case, I see no need to force these arguments onto the new Builder instance [on line 333] - before you step over line 333, the new Builder instance already contains all the necessary data. If you step over line 333 (before applying my patch), you will see that an ugly Builder object is constructed where every superClass attribute is unnecessarily overridden with an identical value in the instance. Actually, the Builder.isDefaultBuildCmd() method always returns 'false' at this point, even when it should return 'true' (because the 'command' attribute is always unnecessarily overridden by in the Builder instance). Once my patch is applied, the BuildCommand no longer duplicates any of the Builder arguments, and is pretty much empty within the dynamic scope of createBuilderFromCommand(). Now, the configuration's Builder object is cloned [on line 313], and the rest of the lines are pretty much a no-op. So, in the end, you get a clean clone of the configuration's Builder object, which is what I would expect this method to return in this use-case.
(In reply to comment #9) > The createBuilder() private method is called from two places - createBuilders() > and createBuilderFromCommand(). The createBuilders() route is taken care of by > the CONTENTS_ACTIVE_CFG_SETTINGS switch. Not exactly. That is CONTENTS_BUILDER_CUSTOMIZATION and that makes more sense. I do not see any other reference to it in the code but I am able to trigger execution of this code path by placing value org.eclipse.cdt.make.core.builderCustomization under the key org.eclipse.cdt.make.core.contents in .project. The effect of the code in createBuilders() in fact is no more than adjusting a couple of attributes in BuildCommand map. It is not clear to me if the map is used elsewhere later on. Is BuildCommand map recreated on each build or possibly can be reused? Can it be proven that the changed map does not change behavior elsewhere?
Not sure I understand what you mean. In my use-case, within the createBuilders() call, the CONTENTS type will always be CONTENTS_ACTIVE_CFG_SETTINGS, because that's what builderBuildArgsMap() (the method changed by my patch) sets it to. So in my use-case, we will never enter createBuilder() from createBuilders(). This is what I meant when I said that this route is taken care of by the CONTENTS_ACTIVE_CFG_SETTINGS switch.
(In reply to comment #9) > But the createBuilderFromCommand() route is very obscure, I agree. This route is actually easier to analyze to me. Since I determined that the map affected by USE_DEFAULT_BUILD_CMD is not actually used below in createBuilder() and the only side effect is changing the origination map. In all createBuilderFromCommand() routes I see a copy of original map is being created so there is guarantee that changes triggered by USE_DEFAULT_BUILD_CMD are discarded. In createBuilders() route I see that BuildManager.basicBuild() uses ProjectDescription.getBuildSpec(false), i.e. returns original map from the original ICommand. That original map is being modified in BuilderFactory.createBuilder(). My guess is that Platform would ensure the safety of ICommand but I do not see the definite proof yet.(In reply to comment #11) > Not sure I understand what you mean. > > In my use-case, within the createBuilders() call, the CONTENTS type will always > be CONTENTS_ACTIVE_CFG_SETTINGS, because that's what builderBuildArgsMap() (the > method changed by my patch) sets it to. So in my use-case, we will never enter > createBuilder() from createBuilders(). This is what I meant when I said that > this route is taken care of by the CONTENTS_ACTIVE_CFG_SETTINGS switch. I am not sure I understand what you are saying either. Obviously we cannot cover your use case only. We need to prove that all the other use cases are not screwed up.
(In reply to comment #12) > (In reply to comment #11) > > Not sure I understand what you mean. > > In my use-case, within the createBuilders() call, the CONTENTS type will > always > > be CONTENTS_ACTIVE_CFG_SETTINGS, because that's what builderBuildArgsMap() > (the > > method changed by my patch) sets it to. So in my use-case, we will never enter > > createBuilder() from createBuilders(). This is what I meant when I said that > > this route is taken care of by the CONTENTS_ACTIVE_CFG_SETTINGS switch. > I am not sure I understand what you are saying either. Obviously we cannot cover > your use case only. We need to prove that all the other use cases are not > screwed up. Please, just bear with me. Keep in mind that you've done analysis but I have not. That part of the code base is not familiar to me and I may be missing some points that would be obvious to you.
(In reply to comment #12) > I am not sure I understand what you are saying either. Obviously we cannot > cover your use case only. We need to prove that all the other use cases are not > screwed up. Sorry, when I said "my use-case", I meant "the use-case affected by my patch"...at least as far as I can see... My patch only touches the builderBuildArgsMap() method, and that method sets the CONTENT to CONTENTS_ACTIVE_CFG_SETTINGS. So, I think that all cases where CONTENT is anything other than CONTENTS_ACTIVE_CFG_SETTINGS would remain unaffected.
(In reply to comment #14) > (In reply to comment #12) > > I am not sure I understand what you are saying either. Obviously we cannot > > cover your use case only. We need to prove that all the other use cases are > not > > screwed up. > Sorry, when I said "my use-case", I meant "the use-case affected by my > patch"...at least as far as I can see... > My patch only touches the builderBuildArgsMap() method, and that method sets the > CONTENT to CONTENTS_ACTIVE_CFG_SETTINGS. So, I think that all cases where > CONTENT is anything other than CONTENTS_ACTIVE_CFG_SETTINGS would remain > unaffected. History of the source file provides some insight. With your change, bug 189302 is back.
Created attachment 210524 [details] Updated patch to address .project file modifications Thanks Andrew, that does explain things. So, the fix for bug 189302 needed to pass data from the "managedbuild" layer down to the "make" layer, and the way that was achieved was to have the "managedbuild" layer persist its data into .project file, and then have the "make" layer read that data using the Platform API. I'm attaching a new patch that fixes bug 189302 differently. This patch reverts the original fix for bug 189302. Instead of channeling the Builder arguments through serializing them into the .project file, I've added new capability to dynamically query the "managedbuild" layer for the Builder arguments through the ConfigurationDataProvider. To revert the original fix, I've also removed the code that serializes the builder-arguments into the .project file each time build-models are syncronized in ConfigurationDataProvider. This means that new projects will have clean, almost empty .project files, but existing projects will forever contain a snapshot of their active-configuration's builder-arguments in their .project files. Let me know if you agree with this. My thinking is - leave that dead data in the users' .project files (because it's no longer used), in order to clean up the CDT code, which is pretty damn convoluted in this area. I guess, the only problem with leaving dead data in users' .project files is that some users might start worrying when they see that their .project contents start looking "stale" over time.
(In reply to comment #16) > Let me know if you agree with this. I looked at it briefly and I think your approach is way better. Would it be possible to add the new method to abstract class CBuildData rather than to introduce a new interface?
Created attachment 210753 [details] Updated patch to address .project file modifications Makes sense, thanks - updated patch attached.
(In reply to comment #18) > Created attachment 210753 [details] > Updated patch to address .project file modifications > Makes sense, thanks - updated patch attached. Can you rebase it on top of current master? I have trouble applying these patches.
Created attachment 210757 [details] Updated patch to address .project file modifications Sorry about that. Updated patch attached. Thanks!
I committed both patches and the test case to master with a few rather cosmetic modifications. Please, doublecheck that I didn't mess it up. Thanks for the patches!
Thanks Andrew!
*** cdt git genie on behalf of Baltasar Belyavsky *** bug 340219: Project metadata files are saved unnecessarily, patch to address .cproject file modifications [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=88bf01e541760bb2aeeb82656f80d5ed9ffbf67b
*** cdt git genie on behalf of Baltasar Belyavsky *** bug 340219: Project metadata files are saved unnecessarily, patch to address .project file modifications [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=6b66aef60bc1a3ee2f5fa862ed63f48721b1c15f
*** cdt git genie on behalf of Baltasar Belyavsky *** bug 340219: Project metadata files are saved unnecessarily, test cases [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=088185204c04f304896540521a423111ae2afc36
