Skip to content

Conversation

@amorellgarcia
Copy link
Contributor

No description provided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amorellgarcia
This piece of code was removed for kepler and then brought back in because juno failed.
Before I accept your pull request I want to be sure this works in juno and kepler (and I still need to find out how I can easily test a pull request)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but this method is now removed from Kepler so I changed it with the "if block" that is below.

That block code, is written folllowing the recommendations from http://wiki.eclipse.org/CDT/ScannerDiscovery61/API
As you can in the paragraph:
"New interface ILanguageSettingsProvidersKeeper for modifying language setting providers, which is implemented by the CConfigurationDescription class. See the function testProjectDescription_ReadWriteDescription() in LanguageSettingsPersistenceProjectTests for example code.:"

This patch also should fix the issue #83

jantje added a commit that referenced this pull request Nov 27, 2013
Fix compilation for CDT 8.2
@jantje jantje merged commit 8d0b6d5 into Sloeber:master Nov 27, 2013
@jantje jantje mentioned this pull request Nov 29, 2013
@jantje
Copy link
Member

jantje commented Nov 29, 2013

Not sure it is related but the indexer in the product doesn't work. Serial is not found.
Seems not everything is properly configured but after configuring it still doesn't work (I mean Serial is not known)
Jantje

@jantje
Copy link
Member

jantje commented Nov 29, 2013

I can now confirm that this fix breaks the indexer in juno.
I downloaded linux64.2013-11-26_02-07-37.tar.gz and untarred (it is not zipped :-( )
That is the version from the build before this change. This version does not have the indexer problem.

@amorellgarcia
Copy link
Contributor Author

I thought that index problem was an unrelated problem. I will investigate it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly the if test for CDT 8.2
I assume having this line in the else should do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that in Kepler that static method no longer exists. Maybe in newer versions there is a separate method to set indexer configuration paths.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

och they didn't make it depricated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ConfigurationDataProvider is located at org.eclipse.cdt.managedbuilder.internal.dataprovider package that is internal. I think that internal classes doesn't need to be deprecated before their removal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know. This code should have been in cdt

@jantje
Copy link
Member

jantje commented Dec 1, 2013

any progress?

@amorellgarcia
Copy link
Contributor Author

I tried with copying the methods (setDefaultLanguageSettingsProviders and getDefaultLanguageSettingsProviders) that were removed in Kepler. In Juno works (of course), but in Kepler there are still issues with the indexer. I think that some settings have changed its location and that there is no simple fix, so maybe you could revert my change until I find a fix.
I will try to find a solution on Wednesday.

@jantje
Copy link
Member

jantje commented Dec 1, 2013

thanks for the update. I'll remove the fix so we are 200% sure the indexer problem in juno is cause by this.
Shall I ask on the cdt list what they would do?
Jantje

@jantje
Copy link
Member

jantje commented Dec 2, 2013

I've tested it and it is really the line "ConfigurationDataProvider.setDefaultLanguageSettingsProviders(project, cfg, cfgDes);" that causes problems.
To test: always start from a blank workspace as it looks as if the behaviour is different if a project has already been made (correctly or not?).

@mattbsoftware
Copy link

Dear Jantje,
I'm sorry to tell you that my husband suddenly passed away 26.NOV.2013. He was very glad he had the pleasure of your support when he started working circuits. We both admired your brains & wit. He is so respected & loved by so many people. I am homered to have been his wife since we were 23. We had such great times, he'll live in my heart infinity.
If you were involved w/ any project together & would like to finish it on his behalf, I can send you whatever you need. He admired your style.
Thank You,
-mtb

Sent from my mobile device.

On Dec 1, 2013, at 6:04 PM, jantje [email protected] wrote:

I've tested it and it is really the line "ConfigurationDataProvider.setDefaultLanguageSettingsProviders(project, cfg, cfgDes);" that causes problems.
To test: always start from a blank workspace as it looks as if the behaviour is different if a project has already been made (correctly or not?).


Reply to this email directly or view it on GitHub.

@jantje
Copy link
Member

jantje commented Dec 2, 2013

Dear
I'm sorry to hear that your husband suddenly passed away. I wish you lots of strength to get through this painful experience. I also wish you a good memory to remember the good times you had together.
From a practical point of view I received all information needed to finish the remaining issues reported by your husband.
Thank you
Jantje

PS you may not be aware but this mail is send due to a subscription on github. All communication is readable on the web. It may be a good idea to unsubscribe.

@jantje
Copy link
Member

jantje commented Dec 3, 2013

When I was debugging the code I noticed that the test
if (cfgDes instanceof ILanguageSettingsProvidersKeeper)
is true in juno. I guess this test is false in kepler. That made me think of a possible solution

Suppose we do

    ILanguageSettingsProvidersKeeper lspk = (ILanguageSettingsProvidersKeeper)cfgDes;
    lspk.setDefaultLanguageSettingsProvidersIds(new String[] {alCfgs.get(i).ToolchainID});
  if (!(cfgDes instanceof ILanguageSettingsProvidersKeeper)) {
      ConfigurationDataProvider.setDefaultLanguageSettingsProviders(project, cfg, cfgDes);
  }

and we compile the code in juno.
IMHO If the test is false in kepler that should work.
If the test is not false we should find another test :-)
What do you think?
Jantje

@amorellgarcia
Copy link
Contributor Author

That test also works in Kepler so we cannot use it to choose between source code.
I pushed new commits to my master branch that seems to solve indexer issues in Kepler and also works in Juno.
I create a new pull request, but please test it before merging. I tested with clean Juno C++ SR2 and Kepler C++ SR2 installations, all seems to work fine when creating new project (indexer included).

@jantje
Copy link
Member

jantje commented Dec 4, 2013

Actually I don't know how to test before merging. I have been googling to find out but didn't find it :-(
Maybe I can test from your master branch?

@amorellgarcia
Copy link
Contributor Author

Yes, you can use my repo as a remote repository to test it.

@jantje
Copy link
Member

jantje commented Dec 4, 2013

I compiled and tested juno in win32 and linux64 and that worked.
The kepler failed but that is probably caused by the fact you do not have the latest pom's.
If you're sure it compiles and runs in kepler you have a go :-)

@jantje
Copy link
Member

jantje commented Dec 7, 2013

@amorellgarcia
Are you going to create a pull request or do I copy the code from your repository?

@amorellgarcia
Copy link
Contributor Author

I wanted to reopen this issue (#86) with a new pull but I didn't know,
so I created a new pull requeset (#97) and you already merged it.
In fact, I rebased my master from your repo a few days ago ;)

El 07/12/13 13:36, jantje escribió:

@amorellgarcia https://github.com/amorellgarcia
Are you going to create a pull request or do I copy the code from your
repository?


Reply to this email directly or view it on GitHub
#86 (comment).

@jantje
Copy link
Member

jantje commented Dec 7, 2013

Ok I'll get the change in.
Edit: seems it is already in there. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants