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

flag-o-matic.eclass: a complete rewrite of get-flag() #1425

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rindeal
Copy link
Contributor

@rindeal rindeal commented May 7, 2016

  • fix case:
    • CFLAGS='-O1 -O2'
    • get-flag '-O*'
    • before -O1
    • now -O2
  • fix case:
    • CFLAGS='-W1,-O1'
    • get-flag '-O*'
    • before -W1,O1
    • now return 1

get-flag march == "i686" syntax still works.

@idella idella added the bugfix label May 7, 2016
@monsieurp
Copy link
Member

@rindeal, hi. you know the drill by now: has this change been reviewed on the gentoo-dev ml?

@rindeal
Copy link
Contributor Author

rindeal commented May 10, 2016

Hi @monsieurp,

https://devmanual.gentoo.org/eclass-writing/ states that such workflow is necessary for major changes, API changes and new eclasses. None of those conditions apply here.

@monsieurp
Copy link
Member

sure but they ought to get reviewed nonetheless by someone from the toolchain team. @blueness @vapier @dirtyepic

@monsieurp
Copy link
Member

also I see bugfix in your commit message. Could you please provide a bug number? thanks!

@monsieurp monsieurp added the assigned PR successfully assigned to the package maintainer(s). label May 10, 2016
@rindeal
Copy link
Contributor Author

rindeal commented May 10, 2016

also I see bugfix in your commit message. Could you please provide a bug number? thanks!

I'm not aware of any bug in bugzilla covering these bugs.

@mgorny
Copy link
Member

mgorny commented May 15, 2016

@rindeal, it is also necessary for widely-used eclasses. Please git send-email --to gentoo-dev@lists.gentoo.org ;-).

@rindeal rindeal force-pushed the eclass/flag-o-matic/get-flag branch 3 times, most recently from e486956 to 75cae1b Compare May 15, 2016 20:14
@gentoo-repo-qa-bot
Copy link
Collaborator

😞 The QA check for this pull request has found the following issues:

Issues persisted from underlying repository state:
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#app-cdr/cdrtools
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#app-cdr/xfburn
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#app-i18n/man-pages-it
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#app-laptop/laptop-mode-tools
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#app-misc/trash-cli
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#app-portage/pfl
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#media-gfx/pinta
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#media-gfx/splash-themes-livedvd
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#net-misc/youtube-viewer
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#net-p2p/classified-ads
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#net-p2p/ncdc
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#sys-boot/yaboot
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#virtual/mda
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#virtual/mta
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#x11-misc/obmenu-generator
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#x11-misc/openbox-menu
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#x11-misc/spacefm
https://qa-reports.gentoo.org/output/gentoo-ci/4cd28fa/output.html#x11-plugins/pidgin-opensteamworks

@gentoo-repo-qa-bot
Copy link
Collaborator

😞 The QA check for this pull request has found the following issues:

Issues persisted from underlying repository state:
https://qa-reports.gentoo.org/output/gentoo-ci/a896b56/output.html#x11-terms/mlterm

@rindeal rindeal force-pushed the eclass/flag-o-matic/get-flag branch from 75cae1b to 4dbbb3f Compare May 16, 2016 12:10
}

fom-tbegin() {
clear-env
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't you just clear-env once and limit the scope of CFLAGS= being set for calls?

Copy link
Member

Choose a reason for hiding this comment

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

(like `CFLAGS='-foo' fom-run ...)

Copy link
Contributor Author

@rindeal rindeal May 18, 2016

Choose a reason for hiding this comment

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

I had this idea as well, unfortunately after I wrote all of it and haven't considered it to be worth the effort to change it.

@rindeal rindeal force-pushed the eclass/flag-o-matic/get-flag branch from 4dbbb3f to 5727b5a Compare May 21, 2016 15:58
@rindeal
Copy link
Contributor Author

rindeal commented May 21, 2016

I've updated the following:

  • more comments, almost every line is now commented
  • more space between expressions for an improved reading experience
  • needle is computed only once
  • comparison now uses a comparison operator instead of a parameter expansion

All in all, the code is now greater than ever.

@rindeal rindeal changed the title flag-o-matic.eclass: bugfix for get-flag() flag-o-matic.eclass: a complete rewrite of get-flag() May 21, 2016
@monsieurp
Copy link
Member

@mgorny is this PR ready for prime time?

@mgorny
Copy link
Member

mgorny commented Jun 5, 2016

Was the final version posted for ml review?

@rindeal
Copy link
Contributor Author

rindeal commented Jun 5, 2016

There you go

@rindeal rindeal closed this Jun 13, 2016
@rindeal rindeal reopened this Mar 18, 2024
@rindeal rindeal force-pushed the eclass/flag-o-matic/get-flag branch from 5727b5a to 4eadc15 Compare March 18, 2024 02:54
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-03-18 03:24 UTC
Newest commit scanned: 4eadc15
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/dea1c76666/output.html

- fix case:
  - `CFLAGS='-O1 -O2'`
  - `get-flag '-O*'`
  - before `-O1`
  - now `-O2`
- fix case:
  - `CFLAGS='-W1,-O1'`
  - `get-flag '-O*'`
  - before `-W1,O1`
  - now return 1

`get-flag march` == "i686" syntax still works.

Signed-off-by: rindeal <dev.rindeal@gmail.com>
Signed-off-by: rindeal <dev.rindeal@gmail.com>
@rindeal rindeal force-pushed the eclass/flag-o-matic/get-flag branch from 4eadc15 to eb168fc Compare March 18, 2024 03:44
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2024-03-18 03:59 UTC
Newest commit scanned: eb168fc
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/51296fa004/output.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s).
Projects
None yet
5 participants