Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated binary parsers and supporting code #135

Merged
merged 2 commits into from
Nov 8, 2022

Conversation

jonahgraham
Copy link
Member

@jonahgraham jonahgraham commented Nov 3, 2022

These binary parsers have been slated for deleting for a while and are replaced with 64-bit compatible
versions.

Some methods still refereneced the 32-bit variants and have been updated to the fully functioning
64-bit variant.

See also Bug 562495

TODO before merging

@mbooth101
Copy link
Member

You also have to update the plugin.xml too, right? https://github.com/eclipse-cdt/cdt/blob/main/core/org.eclipse.cdt.core/plugin.xml#L75

@github-actions
Copy link

github-actions bot commented Nov 3, 2022

Test Results

     578 files       578 suites   44m 57s ⏱️
10 834 tests 10 700 ✔️ 134 💤 0
10 856 runs  10 722 ✔️ 134 💤 0

Results for commit ec83784.

♻️ This comment has been updated with latest results.

@jonahgraham
Copy link
Member Author

You also have to update the plugin.xml too, right? https://github.com/eclipse-cdt/cdt/blob/main/core/org.eclipse.cdt.core/plugin.xml#L75

Yes.

It was rather frustrating that this wasn't caught automatically by deleting the class, it will be with:

@jonahgraham
Copy link
Member Author

That turned out to be more to unravel than I expected. This change now needs #137 merging too.

@jonahgraham
Copy link
Member Author

Test what happens if a project has the old parser referenced and they upgrade to it gone, if there is user visible effect, document in N&N

Nothing bad happens now that #137 is merged. However if a project has unknown binary parsers in it, opening the Binary Parsers tab and saving the results will remove the unknown parsers from the .cproject file.

@mbooth101
Copy link
Member

mbooth101 commented Nov 7, 2022

It looks like some removed IDs are still used in this plugin.xml:

binaryParser="org.eclipse.cdt.core.GNU_ELF;org.eclipse.cdt.core.ELF;org.eclipse.cdt.core.MachO64;org.eclipse.cdt.core.PE;org.eclipse.cdt.core.Cygwin_PE"
id="org.eclipse.linuxtools.cdt.autotools.core.targetPlatform"

Should org.eclipse.cdt.core.PE;org.eclipse.cdt.core.Cygwin_PE be changed here to org.eclipse.cdt.core.PE64;org.eclipse.cdt.core.Cygwin_PE64 ?

Edit: It definitely should change. I can still create new projects with deprecated parsers!!! :-o

@ruspl-afed
Copy link
Member

Nothing bad happens now that #137 is merged. However if a project has unknown binary parsers in it, opening the Binary Parsers tab and saving the results will remove the unknown parsers from the .cproject file.

Not sure what will happen with a lot of existing .cproject files if user will not open "Binary Parsers" tab @jonahgraham

@jonahgraham
Copy link
Member Author

Argh. How long is this piece of string....

@jonahgraham
Copy link
Member Author

Thanks for the reviews - I will address all of it before merging. The fact that projects in CDT 10.7 were still being created with the long deprecated parser ids lead me to want to use a different solution in the IDs. New PS coming soon.

@jonahgraham
Copy link
Member Author

Turns out there is no schema for BinaryParserPage extension point (and a bunch of other ones too). This means that the error checking I added in #136 didn't have any effect on numerous parts of the code.

@jonahgraham
Copy link
Member Author

Not sure what will happen with a lot of existing .cproject files if user will not open "Binary Parsers" tab @jonahgraham

The unknown binary parser IDs are ignored. If no suitable binary parser exists in the project then binaries won't be identified at all.

different solution

I have now pushed my alternate solution - this was what I originally considered, but I liked the idea of just removing the old stuff and progressing forward.

What I have done is left the old IDs in place, but pointing at the new classes. The old IDs are private, so they don't appear in the GUI as an option for users to select anymore.

WDYT?

@ruspl-afed
Copy link
Member

So, it will continue to understand existing (old) identifiers, but will use *64 code. And the existing projects will not be affected at all, correct?

@jonahgraham
Copy link
Member Author

So, it will continue to understand existing (old) identifiers, but will use *64 code. And the existing projects will not be affected at all, correct?

Correct. We'll just live with the old identifiers forever. In hindsight this changing which class is used for the identifier could have been done back in CDT 6.9 instead of creating new identifiers at all.

Copy link
Member

@ruspl-afed ruspl-afed left a comment

Choose a reason for hiding this comment

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

I support the solution that does not impact existing user data

@jonahgraham
Copy link
Member Author

The one failed test is what is tracked/fixed by #128

Copy link
Member

@mbooth101 mbooth101 left a comment

Choose a reason for hiding this comment

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

LGTM

This extension has been in existence since very early days
of CDT, but it never(?) had a schema, so the improvements
done in eclipse-cdt#136 will now show errors in use of this extension.
These binary parsers have been slated for deleting for
a while and are replaced with 64-bit compatible
versions.

Some methods still refereneced the 32-bit variants
and have been updated to the fully functioning
64-bit variant.

The older parser IDs are preserved (forever?) so that
old projects can be opened without needing to do anything.
The IDs now point at the new implementations.

See also Bug 562495
@jonahgraham jonahgraham merged commit 9026f53 into eclipse-cdt:main Nov 8, 2022
@jonahgraham jonahgraham deleted the remove_deprecatd_code branch November 8, 2022 01:58
@jonahgraham jonahgraham added this to the 11.0.0 milestone Nov 8, 2022
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.

None yet

3 participants