Flip configure --enable-arch-native default #2261
Conversation
|
Test plan:
|
|
Backporting to v7 will need to be manual, as release-notes will conflict. |
kinkie
added
S-could-use-an-approval
|
Evidence of a successful slow workflow build in current github environment without -march=native: https://github.com/kinkie/squid/actions/runs/18103427658 |
There was a problem hiding this comment.
Excerpt from a staging test that failed prior to this PR changes:
cfgaux/test-driver: line 112: 92681 Illegal instruction (core dumped) "$@" >> "$log_file" 2>&1
FAIL: tests/testRandomUuid
cfgaux/test-driver: line 112: 92690 Illegal instruction (core dumped) "$@" >> "$log_file" 2>&1
FAIL: tests/testCharacterSet
cfgaux/test-driver: line 112: 92753 Illegal instruction (core dumped) "$@" >> "$log_file" 2>&1
FAIL: tests/testTokenizer
Francesco: Evidence of a successful slow workflow build in current github environment without -march=native: https://github.com/kinkie/squid/actions/runs/18103427658
Evidence of a successful slow workflow build in current GitHub environment without this PR changes: https://github.com/measurement-factory/squid/actions/runs/18106733562
Based on that currently available evidence, we can conclude that these changes are not needed to fix known CI problems or the testing approach itself is inadequate to confirm or reject that need.
Since all three core developers appear to support disabling native builds as dangerous, I suggest focusing on that danger alone as justification for this PR changes.
I adjusted PR description to fix spelling, improve formatting, and simplify/polish wording. Please check.
| <p>The <em>-march=native</em> compiler option is not used by | ||
| default. It is possible to enable it by using the | ||
| <em>--enable-march-native</em> option instead | ||
| </descrip> |
There was a problem hiding this comment.
Backporting to v7 will need to be manual, as release-notes will conflict.
Moreover, after that manual backport, v8 release notes would have to be manually changed as well because the changes will no longer be done during v7-to-v8 upgrade! We should find a way to avoid this extra/unwanted manual work.
On the other hand, we do not want to review PRs like this in the wrong context of a versioned branch, where very different tradeoffs may apply, both in terms of code and in terms of policy. Moreover, a v7 PR would not automatically upport to master either (even if we write a script that facilitates such automated upports) because master is unlikely to have an up-to-date copy of v7 release notes. Thus, some manual work is currently required regardless of the porting direction.
Finally, changes that should be (eventually) backported to more than one versioned branch create a series of similar problems, arguably compounding their negative side effects.
I suggest the following:
-
A temporary shortcut: If PR author believes that PR changes should be automatically backported to vN branch and commits to orchestrating that backporting, they should (a) post a master-based PR, (b) modify release-N.sgml.in file (only) in that PR, and (c) explicitly disclose their intent/commitment in a brief dedicated PR comment attached to the modified release notes file. PR reviewers would have an opportunity to block this temporary shortcut. This shortcut implies that an author who wants to facilitate automtic backporting may have to update master copy of release-N.sgml.in in a prerequisite PR. This shortcut reduces but does not completely eliminate manual labor in otherwise conflict-free backports.
-
Long term: Remove release-N.sgml.in files. When releasing, assemble release notes from individual note files (one per change) that can be easily backported across versions. Instead of changing release-N.sgml.in, the corresponding master PR would add a
doc/release-notes/note-such-and-such-change.mdor similar file. During release, the assembling program can compute the set of upgrade-relevant features by comparing note files available inv(N-1)andvNbranches. To tie all those individual note files together and provide a place for general comments about a release, we would use a doc/release-notes/release-N-template.md or similar template file that the custom assembling program will "instantiate" using existing note files, producing release-N.md or similar.
There was a problem hiding this comment.
IIRC there have been proposals to rework how release notes are built, but nothing emerged that anyone is willing to invest in.
The temporary shortcut doesn't work, v7 and v8 release notes have diverged already, so manual work is unavoidable for now
There was a problem hiding this comment.
IIRC there have been proposals to rework how release notes are built, but nothing emerged that anyone is willing to invest in.
And that sad status quo will naturally continue if all proposals are met with a similar reaction. I am "willing to invest" in my long-term proposal1 if others support it. Do you support it?
The temporary shortcut doesn't work, v7 and v8 release notes have diverged already, so manual work is unavoidable for now
Have they diverged enough to prevent an automated merge in this case? AFAIK, merging does not always require identical base files.
Is there any solution that does not require manual work? If not, then we should be selecting the lesser evil (instead of rejecting all evil).
Footnotes
-
At least as it comes to master/v8 changes. ↩
There was a problem hiding this comment.
I am "willing to invest" in my long-term proposal1 if others support it.
I don't think that proposal would help in this case, but code wins on opinions :)
Have they diverged enough to prevent an automated merge in this case? AFAIK, merging does not always require identical base files
Yes, I tried.
Is there any solution that does not require manual work? If not, then we should be selecting the lesser evil (instead of rejecting all evil).
Not that I can imagine
There was a problem hiding this comment.
I am "willing to invest" in my long-term proposal if others support it.
I don't think that proposal would help in this case, but code wins on opinions :)
Why do you think that moving each individual release change "note" into a dedicated file would not help in cases like this PR? Would not dedicated "note" files prevent/avoid all relevant merge conflicts (because master/v8 PRs would be adding files that do not exist in v7 and, hence, can be merged into v7 cleanly)? I am asking because I would rather not invest in code that is doomed to fail -- if you see a potential serious design flaw, I would like to know about it before doing the implementation legwork!..
Is there any solution that does not require manual work? If not, then we should be selecting the lesser evil (instead of rejecting all evil).
Not that I can imagine
Glad we agree on that.
There was a problem hiding this comment.
Yes, the above is essentially the same "one change per note file" design AFAICT. Do I have your explicit support, in principle, for the proposed long-term changes?
yes
Does C disappear from release-notes published for v7.z (because upgrading from the latest v6 to v7.y does not involve dealing with that backported to v6 change C)
I think it's a rare enough problem that I'm fine with it
There was a problem hiding this comment.
This is why we;
a) should not be backporting UI and feature changes that need release note texts, and
b) automate the minor-release updates within release notes (removing branch diff's) at packaging time, and
c) each branch holds release notes files for older major version/series - to be updated by PRs like this one that are intended/required to backport before initial merge.
Since the intended "first release" is to be v7.2 the release notes edited by this PR should be release-7.sgml.in file and the settings change description start with "From v7.2 ..." to indicate that prior v7 releases are different.
There was a problem hiding this comment.
Alex: Do I have your explicit support, in principle, for the proposed long-term changes?
Francesco: yes
Thank you for your support (in principle).
Amos: This is why we; a) should not be backporting UI and feature changes that need release note texts,
I am not sure what "this" refers to exactly, but I disagree with the implication that some release notes management problems should preclude us from backporting any changes. Instead, we should find a way to reduce release notes management overheads (during backports). My suggestion to generate release notes from individual note files aims to do that (among other things).
and b) automate the minor-release updates within release notes (removing branch diff's) at packaging time,
FWIW, I do not know what automation you have in mind. Is it something similar to the "generate release notes document from individual note files" suggestion being discussed on this thread?
and c) each branch holds release notes files for older major version/series - to be updated by PRs like this one that are intended/required to backport before initial merge.
I hope we can avoid requiring master/v8 PR to update v7 release notes in anticipation of a future backport. Such an update creates a mismatch (until backport actually happens), and there are use cases where we want to merge master/v8 PR first and decide whether to backport it much later, based on, for example, initial deployment experience. It is best to keep any backporting decisions separate from master/v8 PR decisions.
Since the intended "first release" is to be v7.2 the release notes edited by this PR should be
release-7.sgml.infile and the settings change description start with "From v7.2 ..." to indicate that prior v7 releases are different.
I think we should decide what that release-7 notes document in v7.2 release is for:
- If that document is for upgrades from the latest v6 to v7.2, then the document should not say "From v7.2".
- If that document is for upgrades from v7.1 to v7.2, then the document should not contain a lot of other notes that describe changes between v6 and v7.1!
My "generate release notes document from individual files" proposal supports either or even both models -- the generating script should not care much which git branches or tags it compares to find changes, but achieving release notes document scope clarity would obviously help when finalizing this automation.
I suspect we want to start with the simpler "the latest v6 to v7.2" model. With a bit more work, we may even allow the user to specify the two branches/tags that the script should compare. In either case, the generated file should contain all notes relevant for the upgrade from X to Y, without distinguishing/marking intermediate changes like "From v7.2...", although that can be added later, after the bigger problems are solved.
How would you define the scope for that "release-7 notes document in v7.2 release"? What subset of changes (from the first Squid commit to v7.2 release tag) should it annotate?
There was a problem hiding this comment.
Alex: Do I have your explicit support, in principle, for the proposed long-term changes?
Francesco: yesThank you for your support (in principle).
Amos: This is why we; a) should not be backporting UI and feature changes that need release note texts,
I am not sure what "this" refers to exactly,
I mean this entire discussion and planning of functionality to workaround of solve ... a non-problem.
@kinkie simply edited the wrong SGML file. Moving those changes to the release-7.shml.in and these issues go away.
but I disagree with the implication that some release notes management problems should preclude us from backporting any changes. Instead, we should find a way to reduce release notes management overheads (during backports). My suggestion to generate release notes from individual note files aims to do that (among other things).
FYI, the mechanism you propose does nothing to solve the actual problem that v8 release notes should not mention this change. The only release notes which should mention this build change are v7.N texts.
and b) automate the minor-release updates within release notes (removing branch diff's) at packaging time,
FWIW, I do not know what automation you have in mind. Is it something similar to the "generate release notes document from individual note files" suggestion being discussed on this thread?
Yes. Where "individual note files" are the release-N.sgml.in files generated from template.sgml by the automation added in c0789db
Nothing we do can solve the problem of vN+1 release notes being incorrect when a feature is backported. We simply should not be backporting features casually. The older vN is stable (promised not to change UI) already.
and c) each branch holds release notes files for older major version/series - to be updated by PRs like this one that are intended/required to backport before initial merge.
I hope we can avoid requiring master/v8 PR to update v7 release notes in anticipation of a future backport.
We must avoid backporting feature changes to what admin are expecting to be a seamless drop-in upgrade.
Only in exceptional circumstances like this one can we backport, and that comes with pre-planning and reviewer approval of the correct release-N file being edited.
there are use cases where we want to merge master/v8 PR first and decide whether to backport it much later, based on, for example, initial deployment experience. It is best to keep any backporting decisions separate from master/v8 PR decisions.
If the vN really is still "master/vN" then a parallel or followup PR correcting the master branch files should be done and is not related to the backport (which should move the relevant texts).
It comes back to the fact that we have provided a guarantee that Squid X.a to X.z are drop in replacements for each other. It is an exceptional circumstances to break that, the extra work is good for us not to treat the case casually.
Since the intended "first release" is to be v7.2 the release notes edited by this PR should be
release-7.sgml.infile and the settings change description start with "From v7.2 ..." to indicate that prior v7 releases are different.I think we should decide what that
release-7notes document in v7.2 release is for:* If that document is for upgrades from the latest v6 to v7.2, then the document should not say "From v7.2". * If that document is for upgrades from v7.1 to v7.2, then the document should not contain a lot of other notes that describe changes between v6 and v7.1!
It is for both of the above. Ideally there should be no documented changes from v7.1 to v7.2.
But sadly we have this PR and occasional others like it.
PS. you seem to still be trying to solve a problem. Remember there is no problem when we three are all following the Squid Project procedures properly.
There was a problem hiding this comment.
Alex: Do I have your explicit support, in principle, for the proposed long-term changes?
Francesco: yes
Amos: This is why we; a) should not be backporting UI and feature changes that need release note texts,
I am not sure what "this" refers to exactly,
I mean this entire discussion and planning of functionality to workaround of solve ... a non-problem.
The need for error-prone manual synchronization work (as illustrated by recent #2268) and persistent arguments about individual release notes indexing/placement prove that there is a problem here. Even if "editing the right file" were a solution, it would still be a problem.
@kinkie simply edited the wrong SGML file. Moving those changes to the
release-7.shml.inand these issues go away.
The above assertion contradicts Francesco's earlier "Backporting to v7 will need to be manual, as release-notes will conflict" assertion and the actual manual backport of this PR (i.e. #2266): Editing v7 release notes did not allow this PR to be backported automatically. The "problem" cannot be fully solved by editing the right file: The problem is not limited to choosing the right file, and the correct file choice itself may change with time for the same code change (as code changes get backported to earlier and earlier releases).
but I disagree with the implication that some release notes management problems should preclude us from backporting any changes. Instead, we should find a way to reduce release notes management overheads (during backports). My suggestion to generate release notes from individual note files aims to do that (among other things).
FYI, the mechanism you propose does nothing to solve the actual problem that v8 release notes should not mention this change.
I strongly disagree with the premise of this assertion: Until "this change" is backported, v8 release should mention "this change". After "this change" is backported, v8 release notes should stop mentioning it. The proposed solution would accomplish that automatically. Without it, we can only work around a part of the problem by waiving our "this v8 change is not really a v8 change because we plan to backport it to v7 very soon" hands.
The only release notes which should mention this build change are v7.N texts.
... but only after this change lands in v7. And if it were to backported to v6 later, then v7 release notes should stop mentioning it as well. That complication may not apply to this particular change, but other changes were and will be backported to multiple versions. The proposed solution addresses that. "Editing the right file" does not.
and b) automate the minor-release updates within release notes (removing branch diff's) at packaging time,
FWIW, I do not know what automation you have in mind. Is it something similar to the "generate release notes document from individual note files" suggestion being discussed on this thread?
Yes. Where "individual note files" are the
release-N.sgml.infiles generated fromtemplate.sgmlby the automation added in c0789db
That is a "no" answer -- each of my "individual note files" is dedicated to one change. Compiling template.sgml (where multiple notes reside) cannot solve the problems we need to solve. We need to generate (the equivalent of) template.sgml first.
Nothing we do can solve the problem of vN+1 release notes being incorrect when a feature is backported.
What makes you thing that? My "generate release notes" proposal solves that problem AFAICT.
We simply should not be backporting features casually.
Casual or not, backports will continue to happen. We need to deal with them. And during earlier release cycles, backports will be happening often. They are not the only sub-problem we are trying to address here, but they are worth addressing IMO.
If the vN really is still "master/vN" then a parallel or followup PR correcting the master branch files should be done and is not related to the backport (which should move the relevant texts).
The proposed solution eliminates the need for that manual error-prone work.
I think we should decide what that
release-7notes document in v7.2 release is for:* If that document is for upgrades from the latest v6 to v7.2, then the document should not say "From v7.2". * If that document is for upgrades from v7.1 to v7.2, then the document should not contain a lot of other notes that describe changes between v6 and v7.1!It is for both of the above.
I disagree that having a single document that covers two very different upgrade paths is a good idea.
Ideally there should be no documented changes from v7.1 to v7.2. But sadly we have this PR and occasional others like it.
Multiple documents (probably generated on demand rather than static/versioned), one for each upgrade jump would be ideal IMO, but we can start with major upgrades (i.e. the first bullet) and ignore the second bullet needs (or satisfy them at the lower priority).
PS. you seem to still be trying to solve a problem. Remember there is no problem when we three are all following the Squid Project procedures properly.
@yadij, release notes maintenance problems are real and cannot be solved by "following procedures". You may not see those obstacles/complications as real problems (or as problems worth solving), but that disagreement itself does not really matter much in this context. What matters (for now) is whether you are going to block a transition to the proposed scheme, where each change is described in a dedicated "note" file and those individual "note" files are automatically assembled into a release notes document. AFAICT, Francesco supports that idea (in principle). I do not know whether you object to such a change, in principle. Do you?
It's correct they are not needed, if we are okay with the fact of them succeeding being a lucky chance. See https://github.com/squid-cache/squid/actions/workflows/slow.yaml , where each aborted workflow has been re-attempted at least three times by me. Still aborted.
Sure
|
kinkie
added
the
S-waiting-for-reviewer
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Alex: Based on that currently available evidence, we can conclude that these changes are not needed to fix known CI problems or the testing approach itself is inadequate to confirm or reject that need.
Francesco: It's correct they are not needed, if we are okay with the fact of them succeeding being a lucky chance.
I did not assert that this PR changes are not needed. I expect this PR to fix those CI failures, and I support this PR (for several reasons), but that is not the same as having evidence that implies these changes fix those failures.
| <p>The <em>-march=native</em> compiler option is not used by | ||
| default. It is possible to enable it by using the | ||
| <em>--enable-march-native</em> option instead | ||
| </descrip> |
There was a problem hiding this comment.
IIRC there have been proposals to rework how release notes are built, but nothing emerged that anyone is willing to invest in.
And that sad status quo will naturally continue if all proposals are met with a similar reaction. I am "willing to invest" in my long-term proposal1 if others support it. Do you support it?
The temporary shortcut doesn't work, v7 and v8 release notes have diverged already, so manual work is unavoidable for now
Have they diverged enough to prevent an automated merge in this case? AFAIK, merging does not always require identical base files.
Is there any solution that does not require manual work? If not, then we should be selecting the lesser evil (instead of rejecting all evil).
Footnotes
-
At least as it comes to master/v8 changes. ↩
rousskov
added
S-waiting-for-author
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
I forgot to mention that this PR should, IMO, adjust our "layer" files:
- Add
--enable-arch-nativeto the comment that lists "unused" options that depend on the build environment. Look for "Following options should be populated with local settings" in layer-02-maximus.opts and layer-04-noauth-everything.opts. - Add
--disable-arch-nativeto DISTCHECK_CONFIGURE_FLAGS in layer-01-minimal.opts.
I do not insist on these adjustments (because those layer files should have been adjusted earlier), but I recommend them to avoid kicking this can down the road.
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
This comment was marked as outdated.
This comment was marked as outdated.
kinkie
added
S-waiting-for-reviewer
| <p>The <em>-march=native</em> compiler option is not used by | ||
| default. It is possible to enable it by using the | ||
| <em>--enable-march-native</em> option instead | ||
| </descrip> |
There was a problem hiding this comment.
This is why we;
a) should not be backporting UI and feature changes that need release note texts, and
b) automate the minor-release updates within release notes (removing branch diff's) at packaging time, and
c) each branch holds release notes files for older major version/series - to be updated by PRs like this one that are intended/required to backport before initial merge.
Since the intended "first release" is to be v7.2 the release notes edited by this PR should be release-7.sgml.in file and the settings change description start with "From v7.2 ..." to indicate that prior v7 releases are different.
rousskov
removed
S-waiting-for-reviewer
Co-authored-by: Amos Jeffries <yadij@users.noreply.github.com>
Translate from UK English to US English Co-authored-by: Amos Jeffries <yadij@users.noreply.github.com>
kinkie
added
S-waiting-for-reviewer
yadij
left a comment
yadij
left a comment
There was a problem hiding this comment.
Still waiting on:
- the build test layer files to be edited per @rousskov review request #2261 (review)
- the release notes to be moved to the correct (v7) file
done |
yadij
added
M-cleared-for-merge
Flip the default of configure's `--enable-arch-native` option from enabled to disabled. GCC's and clang's `-march=native` argument causes issues in some environments, most notably containers where the compilers' heuristics about CPU feature set may fail. These issues manifest as hard-to-reproduce SIGILL errors in binaries such as `squid` or unit test programs. The new default is safer. Performance-minded administrators still have a convenient option to optimize via `--enable-arch-native`.
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
added
M-merged
yadij
removed
the
S-could-use-an-approval
Flip the default of configure's `--enable-arch-native` option from enabled to disabled. GCC's and clang's `-march=native` argument causes issues in some environments, most notably containers where the compilers' heuristics about CPU feature set may fail. These issues manifest as hard-to-reproduce SIGILL errors in binaries such as `squid` or unit test programs. The new default is safer. Performance-minded administrators still have a convenient option to optimize via `--enable-arch-native`.
kinkie
removed
the
backport-to-v7
4 participants
Flip the default of configure's
--enable-arch-nativeoption fromenabled to disabled.
GCC's and clang's
-march=nativeargument causes issues in someenvironments, most notably containers where the compilers' heuristics
about CPU feature set may fail. These issues manifest as
hard-to-reproduce SIGILL errors in binaries such as
squidor unit testprograms. The new default is safer. Performance-minded administrators
still have a convenient option to optimize via
--enable-arch-native.