Bug 338595

Comment 1 Greg Watson 2011-03-01 17:47:00 EST

It looks like RDT somehow replaces the Environment properties page for the C/C++ Build properties (but not for the main properties view), however it doesn't seem to have an "Append variables to native environment" option.

Comment 2 Greg Watson 2011-04-05 09:50:21 EDT

Once I create a remote project, an file called org.eclipse.cdt.core.prefs is created in .metadata/.plugins/org.eclipse.core.runtime/.settings containing:

	environment/workspace=<?xml version\="1.0" encoding\="UTF-8" standalone\="no"?>\n<environment append\="false" appendContributed\="false"/>\n

This appears to affect the environment of all subsequently created projects.

Comment 3 Greg Watson 2011-04-05 10:02:19 EDT

Isolated to lines 126-129 in RmoetMakefileWizardHandler. Turning off append local environment variables appears to affect all projects not just remote projects.

Comment 4 Chris Recoskie 2011-04-05 10:55:04 EDT

Yeah that's not right... the setting should only be changed for the current project.

John, please look at this ASAP and backport a fix to 4.0.x

Comment 5 John Liu 2011-05-27 13:23:43 EDT

Created attachment 196790 [details]
cvs patch applied to cdt.core and rdt.ui

Chris, can you please review the fix patch?

A small change in the cdt class of UserDefinedEnvironmentSupplier is needed, as I have to increase the visibility of function getEnvironment(Object context) from protected to public. I will open a cdt bugzilla once you confirm the fix.

I think we should also back port the fix into rdt 4.0.*.

Comment 6 Chris Recoskie 2011-05-27 13:55:57 EDT

(In reply to comment #5)
> Created attachment 196790 [details]
> cvs patch applied to cdt.core and rdt.ui
> Chris, can you please review the fix patch?
> A small change in the cdt class of UserDefinedEnvironmentSupplier is needed, as
> I have to increase the visibility of function getEnvironment(Object context)
> from protected to public. I will open a cdt bugzilla once you confirm the fix.
> I think we should also back port the fix into rdt 4.0.*.

Patch seems fine.  I tried it out and remote projects still build fine.

Comment 7 John Liu 2011-05-27 14:05:44 EDT

(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 196790 [details] [details]
> > cvs patch applied to cdt.core and rdt.ui
> > Chris, can you please review the fix patch?
> > A small change in the cdt class of UserDefinedEnvironmentSupplier is needed, as
> > I have to increase the visibility of function getEnvironment(Object context)
> > from protected to public. I will open a cdt bugzilla once you confirm the fix.
> > I think we should also back port the fix into rdt 4.0.*.
> Patch seems fine.  I tried it out and remote projects still build fine.

Thanks, Chris. I tested it to a cdt local project as well.

I also create a bugzilla to track the cdt change. https://bugs.eclipse.org/bugs/show_bug.cgi?id=347492

Comment 8 John Liu 2011-05-27 14:20:59 EDT

Greg, can you please also verify if the patch fixes the problem? Thanks.

Comment 9 Greg Watson 2011-05-27 14:57:28 EDT

Aha! Now I remember why I commented out that line in RemoteMakefileWizardHandler that accidentally got committed. It was to avoid this problem.

Comment 10 John Liu 2011-05-27 15:02:42 EDT

(In reply to comment #9)
> Aha! Now I remember why I commented out that line in
> RemoteMakefileWizardHandler that accidentally got committed. It was to avoid
> this problem.

haha, that is right :) Does the fix work OK for you?

Comment 11 Greg Watson 2011-05-27 15:25:24 EDT

Yes, the problem appears to be fixed.

Comment 12 John Liu 2011-05-27 16:16:34 EDT

Created attachment 196806 [details]
a patch exclusive to rdt code changes

check this rdt exclusive patch into head and ptp_4_0 stream.

Comment 13 John Liu 2011-05-27 16:19:31 EDT

Fixed in head and ptp_4_0 stream.

Comment 14 John Liu 2011-05-27 16:41:11 EDT

Created attachment 196809 [details]
rdt.core patch updated

Actually, UserDefinedEnvironmentSupplier has functions to setAppend by context.
setAppendContributedEnvironment(boolean append, Object context) and setAppendEnvironment(boolean append, Object context), so update the code fix by using these functions instead.

Comment 15 Chris Recoskie 2011-05-31 13:25:12 EDT

The fix for this seems to have broken the fix for  Bug 344380 - Cannot build remote project: "Word too long."

You're going to have to figure out how to fix both.  For now I'm going to back out this fix as IMO the other problem (remote builds not working at all) is worse.

Comment 16 John Liu 2011-06-01 14:29:00 EDT

Created attachment 197133 [details]
new patch

I use a different approach to fix the problem. 

1) Do not turn off append local environment variables when a remote project is created. 
2) When the makefile builder executes build and gets environment variable, turn off append local environment variables first and then get the variables, back up the append value to local environment variables at the end.

With this approach, we need to handle the #2 trick to every place that queries remote environment variables, so far I only updated RemoteMakeBuilder.

Chris/Greg, Please let me know if I have missed anything in this new approach? Does remote tools project also use RemoteMakeBuilder to do the makefile build?

Comment 17 Chris Recoskie 2011-06-01 15:35:38 EDT

All remote makefile builds are done with the RemoteMakeBuilder.

The patch won't necessarily work because it assumes that the build completes successfully in order to revert the values.  The restoration from backup should happen in a finally {...} block.

I'm not really understanding though why we need to modify a global setting in order to resolve this.  The property page for environment variables lets you set whether to append/not append on a per project basis, doesn't it?

Comment 18 John Liu 2011-06-01 17:41:35 EDT

Created attachment 197181 [details]
updated new patch

It is good know that RemoteMakeBuilder is the only builder for a remote makefile project. So it is probably the only place we need to get build configuration environment variables.
 
The restoration to the global append value doesn't depend on the build result. It is in a function to get environment variables, once the variables are resolved, the global append is restored right away. Anyway, to make it very safe, I add try and finally blocks in the function to make sure the append value is restored in any case.

Chris, can you please check the new patch?

Comment 19 Chris Recoskie 2011-06-02 11:19:59 EDT

I still think there is a way to change the append setting on a per-project basis.  At least, there used to be.  I want to see that avenue exhausted before we consider putting this proposed patch in.

Comment 20 Chris Recoskie 2011-06-02 13:09:35 EDT

If you look at org.eclipse.cdt.ui.newui.EnvironmentTab.createControls(Composite) at line 253, you should be able to follow the trail of breadcrumbs and figure out how to set the append setting on the project only.

Here is the snippet:

			public void widgetSelected(SelectionEvent e) {
				if (cfgd != null)
					ce.setAppendEnvironment(false, cfgd);


Also, it is a bit confusing because apparently as soon as you install RDT, the CDT enviroment page gets hidden and there's no way of showing it again.  Bad.  I'll file a bug on that.

Comment 21 John Liu 2011-06-03 15:59:59 EDT

Created attachment 197331 [details]
new fix patch applied to rdt.ui

I have a new fix patch that has the following changes:

1) Get configurations from managedbuildinfo.
2) Only turn off AppendContributedEnvironment, this is the varaiable to control the local system environment, if we also turn off AppendEnvironment, we will lost the remote build environment variables.
3)Move configuration updating from RemoteMakefileWizardHandler to RemoteCommonProjectWizard to cover managed build projects.

Chris, please give another review to the new patch?

Comment 22 John Liu 2011-06-03 16:14:58 EDT

Created attachment 197334 [details]
new patch

Comment 23 John Liu 2011-06-03 16:55:18 EDT

Chris has reviewed the code. Fix it in HEAD and ptp_4_0.