GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-101282: Group and quote BOLT CLI flags by erlend-aasland · Pull Request #104821 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented May 23, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: Enhance the BOLT build process #101282

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@erlend-aasland
pythongh-101282: Group and quote BOLT CLI flags
c14ec85
@erlend-aasland
erlend-aasland requested a review from corona10 as a code owner May 23, 2023 21:27
@erlend-aasland erlend-aasland added skip news needs backport to 3.12 only security fixes labels May 23, 2023
@bedevere-bot bedevere-bot added the awaiting core review label May 23, 2023
@bedevere-bot bedevere-bot mentioned this pull request May 23, 2023
Enhance the BOLT build process #101282
Closed
4 tasks done
corona10
corona10 reviewed May 23, 2023
View reviewed changes

@corona10 corona10 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I am not sure why you want to separate the BOLT options.
Please don't do this, let's maintain options as easy possible.
#104752 is not enough?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10

corona10 commented May 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

When I talked about grouping the BOLT options, it doesn't mean separating them as variables.
It was about sorting.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
corona10
corona10 reviewed May 23, 2023
View reviewed changes
Comment thread configure.ac
[-use-gnu-stack],
[-frame-opt=hot]
)]
["${BOLT_GENERIC_OPTIONS} ${BOLT_OPTIMIZATION_OPTIONS}"]

@corona10 corona10 May 23, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

One pain point of this change is we have to care double quote in multiple places.
(defining BOLT_GENERIC_OPTIONS, defining BOLT_OPTIMIZATION_OPTIONS and BOLT_GENERIC_OPTIONS + BOLT_OPTIMIZATION_OPTIONS)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@erlend-aasland erlend-aasland May 24, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I disagree; I don't see that as a "pain point".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@corona10 corona10 May 24, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I disagree; I don't see that as a "pain point".

Well, My preference is don't care about double quotes as possible.
(This was the root cause of this issue in my guess)
I feel quite uncomfortable maintaining three elements that should be noted by double quotes.
It could be my personal preference. but yeah that's why I said pain point.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
corona10 requested review from Yhg1s and ned-deily May 23, 2023 22:59
@erlend-aasland

erlend-aasland commented May 24, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

I am not sure why you want to separate the BOLT options.
Please don't do this, let's maintain options as easy possible.

Currently, we only apply BOLT options to the optimisation stage. What if we want to add options during the instrumentation stage as well? In that case, won't it make sense to apply the generic flags to both stages? If so, will it not be easier to set the generic flags just once? Correct me if I'm wrong.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@corona10

corona10 commented May 24, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

What if we want to add options during the instrumentation stage as well?


I am not sure at this moment, but my mind is reducing the unnecessary option as possible.
(If my hypothesis is correct, only the binary relocation option will remain)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented May 24, 2023

Copy link
Copy Markdown
Contributor Author

Closing this, as grouping is no longer needed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-aasland closed this May 24, 2023
@erlend-aasland
erlend-aasland deleted the fixup-bolt-config branch May 24, 2023 07:01
@erlend-aasland erlend-aasland mentioned this pull request May 24, 2023
gh-101282: Apply BOLT optimisations to libpython for shared builds #104709
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 left review comments
@Yhg1s Yhg1s Awaiting requested review from Yhg1s
@ned-deily ned-deily Awaiting requested review from ned-deily

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review needs backport to 3.12 only security fixes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@erlend-aasland @corona10 @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.