Community
Participate
Working Groups
Build ID: I20080415-1348 Steps To Reproduce: 1. Open the "CDT build variables" pane in the Eclipse Preferences, under the "C/C++" section. 2. Add a new build variable that overrides an existing build macro defined in a project toolchain macro provider. 3. Rebuild the project using that toolchain to see if the user defined value is used over the MBS defined one. More information: We've noticed that while it is possible to override a MBS CDT Build Variable (as defined by a toolchain's "configurationMacroSupplier" attribute) by re-defining the variable in the Project's user variable list, if the user overrides the variable in the workspace user defined variables (in the Preference Panels, under C/C++/CDT Build Variables), it doesn't override the MBS defined value. I read in the Managed Build System Macro design document (version 1.4) that: "It is possible, but not recommended, that a User-defined macro supplier, an External Extension macro supplier or an Environment Variable Macro supplier override the definition of an MBS pre-defined macro. Overriding MBS pre-defined macros can have unanticipated consequences. " Based on that, is it a bug that only the project level use defined variables are able to override the toolchain's macro values and not the workspace CDT build variables, or is it by design? I find the design document unclear on the subject. It appears that the proper behavior should be to have both the workspace and project user defined macros override the MBS macros values.
Dear Serge, I have several issues below: First, could you please use an concrete example of this bug to describe the steps of reproducing in detail? Second, could you please send me a copy of Managed Build System Macro design document (version 1.4)? I do not have this doc in hand. Thank you very much Zhang Yi
Created attachment 105692 [details] Managed Build Macro Design Document version 1.4 Document as requested
(In reply to comment #1) > Dear Serge, > I have several issues below: > First, could you please use an concrete example of this bug to describe the > steps of reproducing in detail? > > Second, could you please send me a copy of Managed Build System Macro > design document (version 1.4)? I do not have this doc in hand. > > Thank you very much > > Zhang Yi > Here a exact steps to reproduce the issue: 1) Create a new C/C++ Project using the "Executable/Hello World C++Project" template named "Testing". 2) Open the properties of the "Testing" project, and change the artifact name to "Testing_${build_type}" under the "C/C++ Build/ Settings / Build Artifact" section. 3) Rebuild the project, verify that the binary "Testing_none" is produced. 4) Open the project properties of the "Testing" project once more, and go in the section "C/C++ Build / Variables". 5) Create a new variable named "build_type" with the value "foo" 6) Rebuild the project, verify that the binary "Testing_foo" is produced. 7) Open the project properties of the "Testing" project, go in the section "C/C++ Build / Variables" and delete the variable "build_type". 8) Open the user preferences, and open the section "C/C++ / CDT build variables". 9) Create a new variable named "build_type" with the value "bar" 10) Rebuild the project, verify that the binary "Testing_none" is produced, even though "Testing_bar" would be expected.
Hi Serge, Thank you for your information. I have tried to reproduce the bug following your example, but I found there are some differences as follows. 1. When I built the project at the second time as step 6, the binary “Testing_foo” was produced, but at the same time the binary “Testing_none” still existed, it was not deleted or override by “Testing_foo”. So there were two binary files at this time. 2. When I built the project at the third time as step 10, it was shown “make: Nothing to be done for `all'.” on the console, and the build system did noting at all. So there were still two binary files, the “Testing_none” and “Testing_foo”. And then if you build it for the second time, it will generate a binary file “Testing_bar”. Then there are three binary file here. Then I tried several cases, and found something strange. It seems that there is something caches the build variable of the preference, if you change the build variable, and then build the project, the Builder still uses the cached variable, so it makes nothing (since the “Testing_none” is already exist. And if no values of build variable, no matter preference or properties, it will use “none” as the default value. ). If you build again with the new setting (“bar”), the cached value will be changed and the Builder uses the new setting, makes the “Testing_bar”. However, the build variable of the properties of the project has higher priority than the build variable of the preference, so whatever the cached value of build variable is (“Testing_none” or “Testing_bar”), the Builder will use the value of the properties, then make the “Testing_foo” (I found that the value of properties seems not be cached). And if I shut down the application of the plug-in and run it again, the cached value was lost, and the new setting was used (“Testing_bar” generated). These opinions from several test cases, and I will go into the source code to find whether the Builder does so. Serge, you could also make several cases to see if it is so. Thank you! Yi
Dear Serge, As I trace the source code, I find the problem may be that there is an instance of "CProjectDescriptionCache" that caches the Build Settings for the Builder. When we change the value of build variable of the properties of the project, and click "OK" or "Apply", the instance is refreshed right away. But when we change the value of preference of whole workspace, and click "OK" or "Apply", the instance is modified at once, it was modified at the end of the next time building the project. But it did not use the new Build Settings until you build the project for the second time after the modification of the Build Settings, i.e. it means that after you modify the build variable, and build the project, it still use the old build variable (since the instance of "CProjectDescriptionCache" is not modified), and during the building, it finds that the setting is changed, and then refresh the instance, but the result of building is still under the old settings. Then you build the project again, you will find the result is new, since the instance is modified during last building. Actually, I do not know if it is a bug or it is designed to do so. I will contact to Mikhail right now to ask for more info. Thanks, Yi
> Dear Serge, > > As I trace the source code, I find the problem may be that there is an > instance of "CProjectDescriptionCache" that caches the Build Settings for the > Builder. When we change the value of build variable of the properties of the > project, and click "OK" or "Apply", the instance is refreshed right away. But > when we change the value of preference of whole workspace, and click "OK" or > "Apply", the instance is "not" modified at once, it was modified at the end of the > next time building the project. But it did not use the new Build Settings until > you build the project for the second time after the modification of the Build > Settings, i.e. it means that after you modify the build variable, and build the > project, it still use the old build variable (since the instance of > "CProjectDescriptionCache" is not modified), and during the building, it finds > that the setting is changed, and then refresh the instance, but the result of > building is still under the old settings. Then you build the project again, you > will find the result is new, since the instance is modified during last > building. > > Actually, I do not know if it is a bug or it is designed to do so. I will > contact to Mikhail right now to ask for more info. > > Thanks, > > Yi > A printing mistake, really sorry for that. I forget printing the "not" in the sentence below. But when we change the value of preference of whole workspace, and click "OK" or "Apply", the instance is "not" modified at once. Yi
Dear Serge, Please see the mail from Mikhail below. Hi Yi, You are right. According to what Serge says in comment #3, the workspace-level variables do not get propagated to the project settings properly, i.e. as you say, the project settings get updated only after the project build, while they should update right after the workspace settings get applied. This is definitely a bug and it should be fixed. I noticed however that the use-case mentioned in comment #3 differs from that described in the bug description. Serge says there that they are trying to override a variable/macro defined via a tool-chain macro provider with the workspace user variable, which is impossible by design. I’m not sure this is explicitly specified in the design, but the build macros/variables functionality implies that the variables defined for the “more-granular” context always have precedence over those defined at the “less-granular” context, no matter whether those variables are user-defined or not, i.e. the macro “foo” defined at the project level would override the macro “foo” defined at the workspace level, the macro “foo” defined at the configuration level would override the macro “foo” defined at the project level, etc. From my perspective this is the correct behavior taking into account the context-based nature of the build macro definition functionality. From this perspective the bug as it is defined in the description could be treated as invalid. To summarize, here is what I think should be done with regard to this bug: 1. The use-case mentioned in the comment#3 should be fixed. 2. It should be explained for Serge that the use-case mentioned in the bug description is the expected behavior, at least for now. In case Serge disagrees with the current functionality, this could be discussed with the community either in the same bugzilla or via the cdt-det mailing list. 3. Once the two above points are completed the bug could be closed as FIXED Please let me know if you have any further questions regarding working on any of the above points. Regards, Mikhail _____________________________________________ From: Zhang, Yi Y Sent: Monday, June 30, 2008 6:58 AM To: Sennikovsky, Mikhail; He, Yunan; Li, Xiao Feng Cc: MRO China Interns; MRO China Lab Subject: The problem of Bug231581. Dear Mikhail, As I working on Bug231581, I find the problem may be that there is an instance of "CProjectDescriptionCache" that caches the Build Settings for the Builder. When we change the value of build variable of the properties of the project, and click "OK" or "Apply", the instance is refreshed right away. But when we change the value of preference of whole workspace, and click "OK" or "Apply", the instance is not modified at once, it was modified at the end of the next time building the project. But it did not use the new Build Settings until you build the project for the second time after the modification of the Build Settings, i.e. it means that after you modify the build variable, and build the project, it still use the old build variable (since the instance of "CProjectDescriptionCache" is not modified), and during the building, it finds that the setting is changed, and then refresh the instance after building, but the result of building is still under the old settings. Then you build the project again, you will find the result is new (i.e. this result is what we expect at first.), since the instance is modified during last building. Actually, I do not know if it is a bug or it is designed to do so, that the cache does not modify right after the change of preference. Could you please tell me if it is an “invalid” bug or I should have to modify the code to solve the problem? For more info, you can contact me directly or you can read the comments that under the bug. Thank you very much! Yi
Hi Serge, Actually, I am not clear of how to override a toolchain's build variable in the workspace preferences. I do not find the way.Is there a table for toolchain's build variable? I could only find the "Build Value" table in the properties of project or preference of workspace. If the toolchain's build variable could only be changed in the .cproject file? As Mikhail mentioned in the mail, what you said in the description seems different from the example in comment 3. The detail is in Mikhail's mail. If you have other comments please feel free to contact me. The problem in comment 3 is a real bug. I will try to fix it soon. Thanks, Yi
(In reply to comment #8) Yi, > Is there a table for toolchain's > build variable? I could only find the "Build Value" table in the properties of > project or preference of workspace. If the toolchain's build variable could > only be changed in the .cproject file? There is no “pre-defined” list of tool-chain macros, but the tool-integrator can provide a tool-chain build macro supplier that supplies macros for the tool-chain. See the description of the toolChain#configurationMacroSupplier attribute in the “org.eclipse.managedbuilder.core.buildDefinition” extension point description document. As I understand the bug description, Serge is talking about overriding the tool-chain configurationMacroSupplier-defined macros with the user-defined workspace macros, which is currently impossible.
Created attachment 106689 [details] patch for Bug231581
Hi All, I have fixed Bug231581 and made a patch for it. Before attaching the patch, I have run two JUnit cases, "org.eclipse.cdt.core.suite.AutomatedIntegrationSuite" and "org.eclipse.cdt.managedbuilder.tests.suite.AllManagedBuildTests", and found several errors, but I ran back the code and tested, the errors already existed before modification. So Mikhail, could you please help me to review the patch? And if there are no problems, please apply it. Any problems, please feel free to contact me! Thank you very much. Zhang Yi
(In reply to comment #11) Thanks, Yi! I'll have a look..
(In reply to comment #10) > Created an attachment (id=106689) [details] > patch for Bug231581 Hi Yi, The patch looks good. Here are some comments on the patch: 1. The User Var supplier mechanism currently does not initiate a project description refresh on variable change. Making some of its method initiate the refresh while remaining the behavior of others unchanged seems inconsistent. It seems better to me to remain the User Env Var supplier functionality unchanged and instead to initiate the refresh in the UI. BTW the code for this refresh was partially implemented by Oleg (see org.eclipse.cdt.ui.newui.PrefPage_Abstract.doSave(IProgressMonitor monitor)), but for some reason the doSave method does not get invoked. I guess this should be fixed and the bug should be gone. 2. It's better to use the ISProjectDescription.updateProjectDescriptions(IProject[] projects, IProgressMonitor monitor) for project description update (as done by the PrefPage_Abstract.doSave()) instead of explicit iteration through project descriptions. Could you update your patch with the above changes? Thsnks, Mikhail
(In reply to comment #13) > (In reply to comment #10) > > Created an attachment (id=106689) [details] [details] > > patch for Bug231581 > Hi Yi, > > The patch looks good. Here are some comments on the patch: > > 1. The User Var supplier mechanism currently does not initiate a project > description refresh on variable change. Making some of its method initiate the > refresh while remaining the behavior of others unchanged seems inconsistent. It > seems better to me to remain the User Env Var supplier functionality unchanged > and instead to initiate the refresh in the UI. BTW the code for this refresh > was partially implemented by Oleg (see > org.eclipse.cdt.ui.newui.PrefPage_Abstract.doSave(IProgressMonitor monitor)), > but for some reason the doSave method does not get invoked. > I guess this should be fixed and the bug should be gone. > 2. It's better to use the > ISProjectDescription.updateProjectDescriptions(IProject[] projects, > IProgressMonitor monitor) for project description update (as done by the > PrefPage_Abstract.doSave()) instead of explicit iteration through project > descriptions. > > Could you update your patch with the above changes? > > Thsnks, > Mikhail > OK,I will do it soon. Thank you! Yi
Created attachment 106939 [details] The modified patch. Hi Mikhail, I have modified the patch. Please review the new patch. Thank you! Yi
(In reply to comment #15) > Created an attachment (id=106939) [details] > The modified patch. > > Hi Mikhail, > > I have modified the patch. Please review the new patch. Thank you! > > Yi I'll have a look, thanks.
(In reply to comment #15) > Created an attachment (id=106939) [details] > The modified patch. > > Hi Mikhail, > > I have modified the patch. Please review the new patch. Thank you! > > Yi Hi Yi, Here are my comments on the patch: 1. I don’t think that changing and using the PrefPage_Abstract.doSave() in the way you do is the correct approach. Basically the problem is a bit wider than might seem at first glance. If you have a look at the PrefPage_Abstract’s hierarchy you’ll notice two sub-classes one responsible for the Build Environment preference page and another responsible for build variables/macros preference page. Both of them have the problem mentioned in this bug, so doSave() should be called for both of these pages. So I think it’s better to make initiate a description update from the PrefPage_Abstract.performOk. Note that the doSave() invocation should be called only once on OK/APPLY for those two pages instead of calling it once for each page since the project description update might be a costy operation for the case of multiple projects in the workspace. 2. Your patch contains a lot of text-formatting changes that makes it difficult to filter out the real functional changes that you’ve made. Could you please exclude those text-formatting changes? Thanks Mikhail
> Hi Yi, > Here are my comments on the patch: > 1. I don’t think that changing and using the PrefPage_Abstract.doSave() in the > way you do is the correct approach. Basically the problem is a bit wider than > might seem at first glance. If you have a look at the PrefPage_Abstract’s > hierarchy you’ll notice two sub-classes one responsible for the Build > Environment preference page and another responsible for build variables/macros > preference page. Both of them have the problem mentioned in this bug, so > doSave() should be called for both of these pages. So I think it’s better to > make initiate a description update from the PrefPage_Abstract.performOk. Note > that the doSave() invocation should be called only once on OK/APPLY for those > two pages instead of calling it once for each page since the project > description update might be a costy operation for the case of multiple projects > in the workspace. Thank you for your comments, Mikhail. I will try to modify it in your way. I will contact you if there are still problems. > 2. Your patch contains a lot of text-formatting changes that makes it difficult > to filter out the real functional changes that you’ve made. Could you please > exclude those text-formatting changes? I am quite sorry about that! I really found that problem right after I made the patch file. I'm also confused of that. I had just changed several lines of code. I rolled back the code, changed it again, created a new patch, the problem is still there. I had done these several times, but the problem still did not get solved. So I could only attach the one I had made. I will examine this problem then. Thank you! > Thanks > Mikhail Thanks Yi
Created attachment 107281 [details] The new patch. I have modified AbstractCPropertyTab.performOK(). You can have look, Mikhail. I have another mail to you as well. Thank you Yi
(In reply to comment #19) > Created an attachment (id=107281) [details] > The new patch. > > I have modified AbstractCPropertyTab.performOK(). You can have look, Mikhail. I > have another mail to you as well. > > Thank you > > Yi Hi Yi, Here are my comments on the patch: 1. I don’t think that down-casting approach you implemented in the AbstractCPropertyTab.performOK() is the right approach since it contradicts with the basic object-oriented design principles. I guess it would be much more correct to implement a doSave() call directly from the PrefPage_Abstract.performOK(). 2. As I mentioned in my previous comment#17 to this bug, the doSave() should be called only _once_ for both Environment and Macros tab, i.e. it should be called once on each APPLY and OK events for instance of the preferences dialog rather than being called once for each page. The approach you implemented makes it being called once per page. 3. Please make sure the issue is addressed for the APPLY event as well, i.e. performApply() is handled properly. Let me know if you have any questions on any of the above points. Thanks, Mikhail
Created attachment 107411 [details] the new patch Hi Mikhail, Here is the new patch. I modified the code according to your comments. Please have a look at it. Thank you, Yi
(In reply to comment #21) > Created an attachment (id=107411) [details] > the new patch > > Hi Mikhail, > > Here is the new patch. I modified the code according to your comments. > Please have a look at it. > > Thank you, > > Yi Hi Yi, I’ve got two comments on the patch: 1. As I mentioned in my previous comments, the description update should be done once per _dialog_, not once per _page_ as the patch does now. So the performOK() should not always invoke the doSave(), but some additional logic is needed to ensure the per-dialog behavior. See e.g. how setting the project description works in the performOK() for the CDT property pages. Note that this is just an example and I don’t think that the static-variable mechanism should be used for maintaining the per-dialog state, instead the state could be associated with the dialog Shell containing the pages (e.g. via void org.eclipse.swt.widgets.Widget.setData(String key, Object value) and Object getData(String key) of the Shell) thus allowing the per-dialog-state mechanism to be used for multiple dialogs simultaneously. 2. Overriding the performApply() in the way you do is needles since the overriding method exposes exactly the same behavior as the overridden one. Mikhail
Hi Mikhail, Please see my words below. > Hi Yi, > > I’ve got two comments on the patch: > > 1. As I mentioned in my previous comments, the description update should be > done once per _dialog_, not once per _page_ as the patch does now. So the > performOK() should not always invoke the doSave(), but some additional logic is > needed to ensure the per-dialog behavior. See e.g. how setting the project > description works in the performOK() for the CDT property pages. Note that this > is just an example and I don’t think that the static-variable mechanism should > be used for maintaining the per-dialog state, instead the state could be > associated with the dialog Shell containing the pages (e.g. via void > org.eclipse.swt.widgets.Widget.setData(String key, Object value) and Object > getData(String key) of the Shell) thus allowing the per-dialog-state mechanism > to be used for multiple dialogs simultaneously. In fact, I could not quite catch your meaning. Do you mean that: The description update is done in the correct way now. However, we need some additional logic, just like checking the static variable "isChange". But you think the way of using "isChange" is not the best way of maintaining the state of dialog. And you think there may be several dialogs, and we need maintain the state of each dialog, and the static variable way that we use now is not suitable. So you deem that we should maintain the states of each dialog in the Shell of these dialogs, and probably we can save the states in the variable "data" of Widget and maintain it by using setData() and getData(). Maybe we can use the name of dialog as key, while the state as value. Is that right? > 2. Overriding the performApply() in the way you do is needles since the > overriding method exposes exactly the same behavior as the overridden one. Yeah, you are right. I needn't override the performApply(). I would change it back. > Mikhail > Thank you, Yi
(In reply to comment #23) Hi Yi, I mean that the doSave() call is performed from the right place, however the description update is performed on each performOk performed for each CDT preference page (CDT variables and environment), i.e. once per page not per dialog ok/apply handling. This happens because the isChanged mechanism works incorrectly now, i.e. the description update is initiated for each tab from each doSave() call because the isChanged variable gets somehow flagged to true between two doSave calls performed from performOk invocations of different pages. Even if you fix the bug in the isChanged mechanism, the current behavior of performOk will still remain incorrect because it will initiate the description update in the performOK invocation of the _first_ page being called while okPressed handling, while it should be invoked from the performOk of the _last_ page being called (i.e. after all pages (CDT variables and environment) save there data) to ensure that the data from all pages (CDT variables and environment) is reflected in the updated project descriptions. So my main concern is that the approach of one project description update-per-OK/APPLY handling is not implemented yet. Basically I don't insist on any specific way of implementing this approach. My thoughts on the get/setData() mechanism was just to give you a hint on how this could be implemented. Mikhail
(In reply to comment #24) > (In reply to comment #23) > Hi Yi, > > I mean that the doSave() call is performed from the right place, however the > description update is performed on each performOk performed for each CDT > preference page (CDT variables and environment), i.e. once per page not per > dialog ok/apply handling. [Yi]So you think the description update function (in doSave()) should be only executed once per dialog, but not per page as now. And therefore we should add some logic to doSave(). > This happens because the isChanged mechanism works > incorrectly now, i.e. the description update is initiated for each tab from > each doSave() call because the isChanged variable gets somehow flagged to true > between two doSave calls performed from performOk invocations of different > pages. [Yi]You mean we change “isChanged” to true in the performOk() of one page, then go to the next page, even if the second page is not changed, but the “isChanged” is still true, and the description update function will be executed as well. Then how about set “isChanged” false after the execution of the description update function? (That is what I do now. But the description update function might be invoked multiple times as well, and you think that is incorrect. ) > Even if you fix the bug in the isChanged mechanism, the current behavior > of performOk will still remain incorrect because it will initiate the > description update in the performOK invocation of the _first_ page being called > while okPressed handling, [Yi]Yeah, in this case, the performOK is called for each page. And it may be called several times. > while it should be invoked from the performOk of the > _last_ page being called (i.e. after all pages (CDT variables and environment) > save there data) to ensure that the data from all pages (CDT variables and > environment) is reflected in the updated project descriptions. > So my main concern is that the approach of one project description > update-per-OK/APPLY handling is not implemented yet. Basically I don't insist > on any specific way of implementing this approach. My thoughts on the > get/setData() mechanism was just to give you a hint on how this could be > implemented. [Yi]Actually, you think the mechanism now run the description update several times, that’s too costly and incorrect. We should modify the mechanism so that the description update could be executed only once per dialog (after all page save their data). > Mikhail > Thanks, Yi
(In reply to comment #25) Hi Yi, Please see my answers embedded below. Mikhail > [Yi]So you think the description update function (in doSave()) should be only > executed once per dialog, but not per page as now. Yes, the update should be executed once per dialog on each OK/APPLY handling. > And therefore we should add some logic to doSave(). Probably the better approach is to add the logic to the performOk and make the doSave to invoke the description update unconditionally, i.e. to concentrate the logic “once-per-dialog-OK/APPLY” in one place. > [Yi]You mean we change “isChanged” to true in the performOk() of one page, then > go to the next page, even if the second page is not changed, but the > “isChanged” is still true, and the description update function will be executed > as well. Then how about set “isChanged” false after the execution of the > description update function? (That is what I do now. But the description update > function might be invoked multiple times as well, and you think that is > incorrect. ) No, I mean that even after the isChanged is set to false in the performOk of the first page being called it somehow gets set to true again by the time the performOk of the second page is invoked. Here are steps to reproduce: Open preferences dialog -> go to “C/C++->/Environment” page and make some modifications there (e.g. define some variable there) then go to the “C/C++-> Build Variables” page and make some changes there as well (e.g. define some variable there). Now press OK -> the performOk of either environment or variables page is executed, doSave() is called and the description update is performed, isChanged is reset to false. After that the performOk of the other page is invoked – note that the isChanged is set to true by the time the doSave is called and the description update is performed the second time in the doSave(). > [Yi]Yeah, in this case, the performOK is called for each page. And it may be > called several times. No, I mean that if you fix the isChanged behavior to make the above scenario work correctly (i.e. isChanged remains false in the second performOk execution) the behavior of performOk will still be incorrect because the description should be update after the _last_ CDT page saves its data, but not after the _first_ page data save as the current perfomeOk/isChanged behavior implies. > [Yi]Actually, you think the mechanism now run the description update several > times, Yes, see the detailed steps to reproduce specified above. > that’s too costly and incorrect. Yes, the update procedure becomes especially costy when there are multiple projects in the workspace especially when those projects contain lots of settings. > We should modify the mechanism so that > the description update could be executed only once per dialog (after all page > save their data). Yes, the update should be performed once per dialog on each OK/APPLY handling. Regards, Mikhail
