Skip to content
This repository has been archived by the owner on Feb 16, 2025. It is now read-only.

Remove kdbproposal #3201

Merged
merged 10 commits into from
Nov 15, 2020

Conversation

PhilippGackstatter
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Nov 12, 2019

fixes #2993

Removes kdbproposal.h and libproposal as they are no longer needed.

Left to do

Basics

These points need to be fulfilled for every PR:

  • Short descriptions of your changes are in the release notes
    (added as entry in doc/news/_preparation_next_release.md which
    contains _(my name)_)
    Please always add something to the release notes.
  • Details of what you changed are in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, are in the commit messages.
  • The buildservers are happy.
  • The PR is rebased with current master.

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests for my code
  • I fully described what my PR does in the documentation
    (not in the PR description)
  • I fixed all affected documentation
  • I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
  • I updated all meta data (e.g. README.md of plugins and METADATA.ini)
  • I mentioned every code not directly written by me in THIRD-PARTY-LICENSES

Review

Reviewers will usually check the following:

Labels

If you are already Elektra developer:

  • Add the "work in progress" label if you do not want the PR to be reviewed yet.
  • Add the "ready to merge" label if the basics are fulfilled and you also
    say that everything is ready to be merged.

@PhilippGackstatter PhilippGackstatter force-pushed the remove-kdbproposal branch 2 times, most recently from cf82a98 to a6c25a5 Compare November 27, 2019 09:27
@PhilippGackstatter
Copy link
Contributor Author

I removed libproposal as part of this PR because it was empty after removing all functions.

Now dh_install is still looking for the library at install time, but I can't find where it is referenced. Can someone point me to the file I need to modify? Thanks!

@sanssecours
Copy link
Member

Now dh_install is still looking for the library at install time, but I can't find where it is referenced. Can someone point me to the file I need to modify? Thanks!

I think you need to remove this line:

usr/lib/*/libelektra-proposal.so*

in the debian branch.

PhilippGackstatter pushed a commit that referenced this pull request Nov 27, 2019
@markus2330
Copy link
Contributor

Thank you for helping out!

@mpranj
Copy link
Member

mpranj commented Nov 5, 2020

Hi 👋, is there a reason why we did not merge this yet?

Does it depend on the Keyname overhaul or can it be merged independently?

@PhilippGackstatter
Copy link
Contributor Author

Does it depend on the Keyname overhaul or can it be merged independently?

I believe it can be merged independently, but I've forgotten the details. I can look into this if this is still relevant. I'm not sure how much of #2993 was actually taken care of here.

@mpranj
Copy link
Member

mpranj commented Nov 5, 2020

I can look into this if this is still relevant.

It's not high priority, but seems like some work already went into it and it looks almost finished. I have not checked though if everything from #2993 has been implemented.

@PhilippGackstatter
Copy link
Contributor Author

I've rebased and looked at what's missing.
It's actually just elektraKeySetName that's still in kdbproposal.h but it would be removed by #3555. After that, kdbproposal.h is empty and can be removed.
And then there is elektraKsPopAtCursor which is declared in kdbprivate.h and implemented in src/libs/elektra/proposal.c and would be moved as part of #3189. This wasn't part of libproposal anyway and I think the PR that implements that can move the function and delete this proposal.c file, and then that would be the last proposal-thing.

So I think this can be merged as is, as the rest is better handled in other PRs.

markus2330 pushed a commit that referenced this pull request Nov 13, 2020
@markus2330
Copy link
Contributor

Thank you for looking into it! I fixed now the Debian branch to not include the proposal library and header file.

@PhilippGackstatter
Copy link
Contributor Author

PhilippGackstatter commented Nov 13, 2020

No idea what's wrong with the ruby plugin. It fails on all darwin builds. Nothing from this PR should affect the plugin in any way, so my guess would be its an external issue. It seems there have been some issues in the past with SWIG and ruby. Do you have any ideas by chance @mpranj @markus2330?

This was referenced Nov 14, 2020
@markus2330
Copy link
Contributor

Yes, ../src/plugins/ruby/ruby.cpp:39:15: error: anonymous non-C-compatible type given name for linkage purposes by typedef declaration; add a tag name here [-Werror,-Wnon-c-typedef-for-linkage] also seems to me quite unrelated. Somehow Qnil or VALUE get wrongly defined? I created #3559.

@mpranj If the same error also occurs on master, we can probably merge this.

@mpranj mpranj merged commit 6bb3e2c into ElektraInitiative:master Nov 15, 2020
@mpranj
Copy link
Member

mpranj commented Nov 15, 2020

Thank you so much @PhilippGackstatter, great to finally have this cleaned up!

I confirmed the ruby problem exists on master too (unfortunately).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove kdbproposal.h
4 participants