Don't clobber existing CMAKE_CXX_FLAGS_RELEASE

[swolchok]

We were likely losing -DNDEBUG. Preserve our current intent to clobber the -O flag in particular, though.

[swolchok]

Stack from ghstack (oldest at bottom):

• Don't default to -fno-exceptions -fno-rtti #7746
• Don't repeat default for CMAKE_CXX_FLAGS_DEBUG #7743
• ->Don't clobber existing CMAKE_CXX_FLAGS_RELEASE #7742

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7742

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 37114d8 with merge base cd0e584 ():

NEW FAILURE - The following job has failed:

• pull / test-phi-3-mini-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 018c3c993574ac4d9c83f31d984503b3dfc194f9c94bae809f16dd1f3d399f09 /exec failed with exit code 2

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[dbort]

It's pretty typical to put the overridable CMAKE_CXX_FLAGS_RELEASE last so it can override -Os/-O2 if users want to; the new order will always override whatever CMAKE_CXX_FLAGS_RELEASE says. But if that's the intent (like the PR description says), then this seems to do it.

But since this is an unusual pattern, please update the comments on these two cases to clarify that we're intentionally overriding the -O flag so that someone doesn't come back and "fix" it.

[swolchok]

someone doesn't come back and "fix" it.

I would be happy for someone to come back and change this further; these two OPTIMIZE_SIZE changes were to preserve existing behavior.

Before: we clobber all of CMAKE_CXX_FLAGS_RELEASE on line 152 above.
After: we only clobber the -O setting with -Os or -O2.

I think our setup is deeply weird and OPTIMIZE_SIZE shouldn't exist; I am just following general good practice by sending an incremental diff that makes a small amount of forward progress rather than attempting to fix everything all at once.

[dbort]

After: we only clobber the -O setting with -Os or -O2.

Agreed that it's an improvement. But someone a year from now who's only looking at this line might think "why is -Os last? I'll move it back before the ${CMAKE_CXX_FLAGS_RELEASE}". A comment would help avoid that.

[swolchok]

I'm hesitant to enshrine behavior I think is wrong and simply haven't gotten around to fixing yet.

[swolchok]

We were likely losing -DNDEBUG. Preserve our current intent to clobber the -O flag in particular, though.

[swolchok]

Stack from ghstack (oldest at bottom):

• Don't default to -fno-exceptions -fno-rtti #7746
• Don't repeat default for CMAKE_CXX_FLAGS_DEBUG #7743
• ->Don't clobber existing CMAKE_CXX_FLAGS_RELEASE #7742

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7742

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 37114d8 with merge base cd0e584 ():

NEW FAILURE - The following job has failed:

• pull / test-phi-3-mini-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 018c3c993574ac4d9c83f31d984503b3dfc194f9c94bae809f16dd1f3d399f09 /exec failed with exit code 2

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[dbort]

It's pretty typical to put the overridable CMAKE_CXX_FLAGS_RELEASE last so it can override -Os/-O2 if users want to; the new order will always override whatever CMAKE_CXX_FLAGS_RELEASE says. But if that's the intent (like the PR description says), then this seems to do it.

But since this is an unusual pattern, please update the comments on these two cases to clarify that we're intentionally overriding the -O flag so that someone doesn't come back and "fix" it.

[swolchok]

someone doesn't come back and "fix" it.

I would be happy for someone to come back and change this further; these two OPTIMIZE_SIZE changes were to preserve existing behavior.

Before: we clobber all of CMAKE_CXX_FLAGS_RELEASE on line 152 above.
After: we only clobber the -O setting with -Os or -O2.

I think our setup is deeply weird and OPTIMIZE_SIZE shouldn't exist; I am just following general good practice by sending an incremental diff that makes a small amount of forward progress rather than attempting to fix everything all at once.

[dbort]

After: we only clobber the -O setting with -Os or -O2.

Agreed that it's an improvement. But someone a year from now who's only looking at this line might think "why is -Os last? I'll move it back before the ${CMAKE_CXX_FLAGS_RELEASE}". A comment would help avoid that.

[swolchok]

I'm hesitant to enshrine behavior I think is wrong and simply haven't gotten around to fixing yet.

[swolchok]

We were likely losing -DNDEBUG. Preserve our current intent to clobber the -O flag in particular, though.

[swolchok]

Stack from ghstack (oldest at bottom):

• Don't default to -fno-exceptions -fno-rtti #7746
• Don't repeat default for CMAKE_CXX_FLAGS_DEBUG #7743
• ->Don't clobber existing CMAKE_CXX_FLAGS_RELEASE #7742

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7742

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 37114d8 with merge base cd0e584 ():

NEW FAILURE - The following job has failed:

• pull / test-phi-3-mini-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 018c3c993574ac4d9c83f31d984503b3dfc194f9c94bae809f16dd1f3d399f09 /exec failed with exit code 2

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[dbort]

It's pretty typical to put the overridable CMAKE_CXX_FLAGS_RELEASE last so it can override -Os/-O2 if users want to; the new order will always override whatever CMAKE_CXX_FLAGS_RELEASE says. But if that's the intent (like the PR description says), then this seems to do it.

But since this is an unusual pattern, please update the comments on these two cases to clarify that we're intentionally overriding the -O flag so that someone doesn't come back and "fix" it.

[swolchok]

someone doesn't come back and "fix" it.

I would be happy for someone to come back and change this further; these two OPTIMIZE_SIZE changes were to preserve existing behavior.

Before: we clobber all of CMAKE_CXX_FLAGS_RELEASE on line 152 above.
After: we only clobber the -O setting with -Os or -O2.

I think our setup is deeply weird and OPTIMIZE_SIZE shouldn't exist; I am just following general good practice by sending an incremental diff that makes a small amount of forward progress rather than attempting to fix everything all at once.

[dbort]

After: we only clobber the -O setting with -Os or -O2.

Agreed that it's an improvement. But someone a year from now who's only looking at this line might think "why is -Os last? I'll move it back before the ${CMAKE_CXX_FLAGS_RELEASE}". A comment would help avoid that.

[swolchok]

I'm hesitant to enshrine behavior I think is wrong and simply haven't gotten around to fixing yet.

[swolchok]

We were likely losing -DNDEBUG. Preserve our current intent to clobber the -O flag in particular, though.

[swolchok]

Stack from ghstack (oldest at bottom):

• Don't default to -fno-exceptions -fno-rtti #7746
• Don't repeat default for CMAKE_CXX_FLAGS_DEBUG #7743
• ->Don't clobber existing CMAKE_CXX_FLAGS_RELEASE #7742

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7742

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 37114d8 with merge base cd0e584 ():

NEW FAILURE - The following job has failed:

• pull / test-phi-3-mini-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 018c3c993574ac4d9c83f31d984503b3dfc194f9c94bae809f16dd1f3d399f09 /exec failed with exit code 2

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[dbort]

It's pretty typical to put the overridable CMAKE_CXX_FLAGS_RELEASE last so it can override -Os/-O2 if users want to; the new order will always override whatever CMAKE_CXX_FLAGS_RELEASE says. But if that's the intent (like the PR description says), then this seems to do it.

But since this is an unusual pattern, please update the comments on these two cases to clarify that we're intentionally overriding the -O flag so that someone doesn't come back and "fix" it.

[swolchok]

someone doesn't come back and "fix" it.

I would be happy for someone to come back and change this further; these two OPTIMIZE_SIZE changes were to preserve existing behavior.

Before: we clobber all of CMAKE_CXX_FLAGS_RELEASE on line 152 above.
After: we only clobber the -O setting with -Os or -O2.

I think our setup is deeply weird and OPTIMIZE_SIZE shouldn't exist; I am just following general good practice by sending an incremental diff that makes a small amount of forward progress rather than attempting to fix everything all at once.

[dbort]

After: we only clobber the -O setting with -Os or -O2.

Agreed that it's an improvement. But someone a year from now who's only looking at this line might think "why is -Os last? I'll move it back before the ${CMAKE_CXX_FLAGS_RELEASE}". A comment would help avoid that.

[swolchok]

I'm hesitant to enshrine behavior I think is wrong and simply haven't gotten around to fixing yet.

[swolchok]

We were likely losing -DNDEBUG. Preserve our current intent to clobber the -O flag in particular, though.

[swolchok]

Stack from ghstack (oldest at bottom):

• Don't default to -fno-exceptions -fno-rtti #7746
• Don't repeat default for CMAKE_CXX_FLAGS_DEBUG #7743
• ->Don't clobber existing CMAKE_CXX_FLAGS_RELEASE #7742

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7742

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 37114d8 with merge base cd0e584 ():

NEW FAILURE - The following job has failed:

• pull / test-phi-3-mini-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 018c3c993574ac4d9c83f31d984503b3dfc194f9c94bae809f16dd1f3d399f09 /exec failed with exit code 2

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[dbort]

It's pretty typical to put the overridable CMAKE_CXX_FLAGS_RELEASE last so it can override -Os/-O2 if users want to; the new order will always override whatever CMAKE_CXX_FLAGS_RELEASE says. But if that's the intent (like the PR description says), then this seems to do it.

But since this is an unusual pattern, please update the comments on these two cases to clarify that we're intentionally overriding the -O flag so that someone doesn't come back and "fix" it.

[swolchok]

someone doesn't come back and "fix" it.

I would be happy for someone to come back and change this further; these two OPTIMIZE_SIZE changes were to preserve existing behavior.

Before: we clobber all of CMAKE_CXX_FLAGS_RELEASE on line 152 above.
After: we only clobber the -O setting with -Os or -O2.

I think our setup is deeply weird and OPTIMIZE_SIZE shouldn't exist; I am just following general good practice by sending an incremental diff that makes a small amount of forward progress rather than attempting to fix everything all at once.

[dbort]

After: we only clobber the -O setting with -Os or -O2.

Agreed that it's an improvement. But someone a year from now who's only looking at this line might think "why is -Os last? I'll move it back before the ${CMAKE_CXX_FLAGS_RELEASE}". A comment would help avoid that.

[swolchok]

I'm hesitant to enshrine behavior I think is wrong and simply haven't gotten around to fixing yet.

[swolchok]

We were likely losing -DNDEBUG. Preserve our current intent to clobber the -O flag in particular, though.

[swolchok]

Stack from ghstack (oldest at bottom):

• Don't default to -fno-exceptions -fno-rtti #7746
• Don't repeat default for CMAKE_CXX_FLAGS_DEBUG #7743
• ->Don't clobber existing CMAKE_CXX_FLAGS_RELEASE #7742

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7742

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 37114d8 with merge base cd0e584 ():

NEW FAILURE - The following job has failed:

• pull / test-phi-3-mini-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 018c3c993574ac4d9c83f31d984503b3dfc194f9c94bae809f16dd1f3d399f09 /exec failed with exit code 2

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[dbort]

It's pretty typical to put the overridable CMAKE_CXX_FLAGS_RELEASE last so it can override -Os/-O2 if users want to; the new order will always override whatever CMAKE_CXX_FLAGS_RELEASE says. But if that's the intent (like the PR description says), then this seems to do it.

But since this is an unusual pattern, please update the comments on these two cases to clarify that we're intentionally overriding the -O flag so that someone doesn't come back and "fix" it.

[swolchok]

someone doesn't come back and "fix" it.

I would be happy for someone to come back and change this further; these two OPTIMIZE_SIZE changes were to preserve existing behavior.

Before: we clobber all of CMAKE_CXX_FLAGS_RELEASE on line 152 above.
After: we only clobber the -O setting with -Os or -O2.

I think our setup is deeply weird and OPTIMIZE_SIZE shouldn't exist; I am just following general good practice by sending an incremental diff that makes a small amount of forward progress rather than attempting to fix everything all at once.

[dbort]

After: we only clobber the -O setting with -Os or -O2.

Agreed that it's an improvement. But someone a year from now who's only looking at this line might think "why is -Os last? I'll move it back before the ${CMAKE_CXX_FLAGS_RELEASE}". A comment would help avoid that.

[swolchok]

I'm hesitant to enshrine behavior I think is wrong and simply haven't gotten around to fixing yet.

[swolchok]

We were likely losing -DNDEBUG. Preserve our current intent to clobber the -O flag in particular, though.

[swolchok]

Stack from ghstack (oldest at bottom):

• Don't default to -fno-exceptions -fno-rtti #7746
• Don't repeat default for CMAKE_CXX_FLAGS_DEBUG #7743
• ->Don't clobber existing CMAKE_CXX_FLAGS_RELEASE #7742

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7742

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 37114d8 with merge base cd0e584 ():

NEW FAILURE - The following job has failed:

• pull / test-phi-3-mini-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 018c3c993574ac4d9c83f31d984503b3dfc194f9c94bae809f16dd1f3d399f09 /exec failed with exit code 2

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[dbort]

It's pretty typical to put the overridable CMAKE_CXX_FLAGS_RELEASE last so it can override -Os/-O2 if users want to; the new order will always override whatever CMAKE_CXX_FLAGS_RELEASE says. But if that's the intent (like the PR description says), then this seems to do it.

But since this is an unusual pattern, please update the comments on these two cases to clarify that we're intentionally overriding the -O flag so that someone doesn't come back and "fix" it.

[swolchok]

someone doesn't come back and "fix" it.

I would be happy for someone to come back and change this further; these two OPTIMIZE_SIZE changes were to preserve existing behavior.

Before: we clobber all of CMAKE_CXX_FLAGS_RELEASE on line 152 above.
After: we only clobber the -O setting with -Os or -O2.

I think our setup is deeply weird and OPTIMIZE_SIZE shouldn't exist; I am just following general good practice by sending an incremental diff that makes a small amount of forward progress rather than attempting to fix everything all at once.

[dbort]

After: we only clobber the -O setting with -Os or -O2.

Agreed that it's an improvement. But someone a year from now who's only looking at this line might think "why is -Os last? I'll move it back before the ${CMAKE_CXX_FLAGS_RELEASE}". A comment would help avoid that.

[swolchok]

I'm hesitant to enshrine behavior I think is wrong and simply haven't gotten around to fixing yet.

[swolchok]

We were likely losing -DNDEBUG. Preserve our current intent to clobber the -O flag in particular, though.

[swolchok]

Stack from ghstack (oldest at bottom):

• Don't default to -fno-exceptions -fno-rtti #7746
• Don't repeat default for CMAKE_CXX_FLAGS_DEBUG #7743
• ->Don't clobber existing CMAKE_CXX_FLAGS_RELEASE #7742

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7742

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 37114d8 with merge base cd0e584 ():

NEW FAILURE - The following job has failed:

• pull / test-phi-3-mini-runner-linux / linux-job (gh)
  RuntimeError: Command docker exec -t 018c3c993574ac4d9c83f31d984503b3dfc194f9c94bae809f16dd1f3d399f09 /exec failed with exit code 2

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[dbort]

It's pretty typical to put the overridable CMAKE_CXX_FLAGS_RELEASE last so it can override -Os/-O2 if users want to; the new order will always override whatever CMAKE_CXX_FLAGS_RELEASE says. But if that's the intent (like the PR description says), then this seems to do it.

But since this is an unusual pattern, please update the comments on these two cases to clarify that we're intentionally overriding the -O flag so that someone doesn't come back and "fix" it.

[swolchok]

someone doesn't come back and "fix" it.

I would be happy for someone to come back and change this further; these two OPTIMIZE_SIZE changes were to preserve existing behavior.

Before: we clobber all of CMAKE_CXX_FLAGS_RELEASE on line 152 above.
After: we only clobber the -O setting with -Os or -O2.

I think our setup is deeply weird and OPTIMIZE_SIZE shouldn't exist; I am just following general good practice by sending an incremental diff that makes a small amount of forward progress rather than attempting to fix everything all at once.

[dbort]

After: we only clobber the -O setting with -Os or -O2.

Agreed that it's an improvement. But someone a year from now who's only looking at this line might think "why is -Os last? I'll move it back before the ${CMAKE_CXX_FLAGS_RELEASE}". A comment would help avoid that.

[swolchok]

I'm hesitant to enshrine behavior I think is wrong and simply haven't gotten around to fixing yet.