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.

Sign up for GitHub

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

Jump to bottom

net-misc/ncp: remove diet use flag, fix bug #737254 #17162

Closed
wants to merge 2 commits into from
Closed

net-misc/ncp: remove diet use flag, fix bug #737254 #17162

wants to merge 2 commits into from

Conversation

mm1ke
Copy link
Contributor

@mm1ke mm1ke commented Aug 18, 2020

Hi,

this is a simple update to r1 which just updates the sed command to not use colons in order to avoid problems with -falign-functions=32:25:16.

1c1
< # Copyright 1999-2019 Gentoo Authors
---
> # Copyright 1999-2020 Gentoo Authors
23c23
<       sed -e '/^ncp:/,+5s:strip:#strip:' \
---
>       sed -e '/^ncp:/,+5s|strip|#strip|' \

Package-Manager: Portage-3.0.2, Repoman-2.3.23
Signed-off-by: Michael Mair-Keimberger m.mairkeimberger@gmail.com

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Aug 18, 2020
@mm1ke
Copy link
Contributor Author

mm1ke commented Aug 18, 2020

Please do not merge yet, i want to add another fix for bug: https://bugs.gentoo.org/737254

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-08-18 17:01 UTC
Newest commit scanned: 8ae198d
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/1898990947/output.html

@SoapGentoo
Copy link
Member

  1. Please use a patch instead of a sed
  2. Just remove the diet USE flag. I'm not convinced by adding special flags to appease someone like fefe, and I don't see the massive win here.

@mm1ke mm1ke changed the title net-misc/ncp: avoid using colons in sed net-misc/ncp: remove diet use flag, fix bug #737254 Aug 19, 2020
@mm1ke mm1ke changed the title net-misc/ncp: remove diet use flag, fix bug #737254 net-misc/ncp: remove diet use flag, fix bug #737254 [please reassign] Aug 19, 2020
@mm1ke
Copy link
Contributor Author

mm1ke commented Aug 19, 2020 •
edited

  1. Please use a patch instead of a sed

Sure, done.

  1. Just remove the diet USE flag. I'm not convinced by adding special flags to appease someone like fefe, and I don't see the massive win here.

Sure, this makes for me easier too :)

@gentoo-bot gentoo-bot changed the title net-misc/ncp: remove diet use flag, fix bug #737254 [please reassign] net-misc/ncp: remove diet use flag, fix bug #737254 Aug 19, 2020
@gentoo-bot
Copy link

Pull Request assignment

Submitter: @mm1ke
Areas affected: ebuilds
Packages affected: net-misc/ncp

net-misc/ncp: mmk[at]levelnine.at, @gentoo/proxy-maint

Linked bugs

Bugs linked: 737254

In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of Conduct ● Copyright policy (expl.) ● Devmanual ● GitHub PRs ● Proxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. and removed assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Aug 19, 2020
thesamesam
thesamesam reviewed Aug 19, 2020
View reviewed changes
net-misc/ncp/files/ncp-1.2.4-fix-makefile.patch Outdated
$(CC) $< -o $@ -I. $(CFLAGS) $(LDFLAGS) $(LDLIBS) `cat libsocket`
ifeq ($(DEBUG),)
- strip -R .note -R .comment ncp || strip ncp
+ #strip -R .note -R .comment ncp || strip ncp
Copy link
Member

Choose a reason for hiding this comment

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

No need to comment out with a patch - we can see what we removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, i don't understand? I shouldn't use a patch here? (and use sed instead again?)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, what I'm saying is... if you remove a line with a patch, you can see the missing line, right?

There's no need to put a '#' in, just drop the line, because we still get to see what the old line was. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh.. i understand now :)
i've updated the patch, now it removes the lines (and also the ifeq .. endif since it would be empty anyway)

net-misc/ncp/files/ncp-1.2.4-fix-makefile.patch
@@ -0,0 +1,11 @@
--- ncp-1.2.4/GNUmakefile 2005-05-13 19:17:17.000000000 +0200
Copy link
Member

Choose a reason for hiding this comment

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

I usually like to put a header with a Gentoo bug ref at least.

Copy link
Member

Choose a reason for hiding this comment

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

ping

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've updated the header, but didn't add a bug reference since i couldn't find one. Hope thats enough.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-08-19 17:46 UTC
Newest commit scanned: 1845399
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/288aac799d/output.html

@mm1ke
Copy link
Contributor Author

mm1ke commented Aug 28, 2020

@thesamesam
Just rebased and updated the patch with a header. I still not sure about your first comment, please have a look :)

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-08-28 16:55 UTC
Newest commit scanned: 9df2688
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/bfd4ba359d/output.html

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-09-04 18:36 UTC
Newest commit scanned: bf8ddcc
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/f34a0fa0fd/output.html

@mm1ke
net-misc/ncp: remove diet use flag, fix bug #737254
77b9378 
Package-Manager: Portage-3.0.2, Repoman-2.3.23
Signed-off-by: Michael Mair-Keimberger <m.mairkeimberger@gmail.com>
Closes: https://bugs.gentoo.org/737254
@mm1ke
net-misc/ncp: remove old
31101c5 
Package-Manager: Portage-3.0.2, Repoman-2.3.23
Signed-off-by: Michael Mair-Keimberger <m.mairkeimberger@gmail.com>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-09-12 08:21 UTC
Newest commit scanned: 31101c5
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/7ffbb13630/output.html

@mm1ke
Copy link
Contributor Author

mm1ke commented Sep 17, 2020

@thesamesam @SoapGentoo
Would be nice if anyone could push this :)
Thanks.

@thesamesam
Copy link
Member

Done, thank you!

@mm1ke mm1ke deleted the ncpp-va87EzEN branch October 28, 2020 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thesamesam thesamesam thesamesam left review comments

Assignees
No one assigned
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR.
Projects
None yet
Milestone
No milestone
5 participants
@mm1ke @gentoo-repo-qa-bot @SoapGentoo @gentoo-bot @thesamesam