Skip to content

Conversation

@akoeplinger
Copy link
Member

@akoeplinger akoeplinger commented Nov 29, 2023

While looking at #95405 I noticed that we didn't fail the build when the cmake configure step failed but tried to run the build regardless.

The reason is because in ece10ad another command popd was added after the cmake invocation which overwrote the exit code so build-commons.sh didn't know cmake failed.

We can use the -S and -B flags to set the directories instead again now that we use a new enough minimum CMake version.

While looking at dotnet#95405 I noticed that we didn't fail the build when the cmake configure step failed but tried to run the build regardless.
The reason is because in ece10ad another command `popd` was added after the cmake invocation which overwrote the exit code so build-commons.sh didn't know cmake failed.
@ghost
Copy link

ghost commented Nov 29, 2023

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

While looking at #95405 I noticed that we didn't fail the build when the cmake configure step failed but tried to run the build regardless.

The reason is because in ece10ad another command popd was added after the cmake invocation which overwrote the exit code so build-commons.sh didn't know cmake failed.

Author: akoeplinger
Assignees: -
Labels:

area-Infrastructure

Milestone: -

@ghost ghost assigned akoeplinger Nov 29, 2023
@jkoritzinsky
Copy link
Member

Alternatively, can we change the script to use the -S and -B flags so we don't need the pushd/pops in the first place?

@akoeplinger
Copy link
Member Author

@jkoritzinsky I didn't do that because of

# We have to be able to build with CMake 3.6.2, so we can't use the -S or -B options
pushd "$2"
but we're using a newer minimum CMake version now which supports those flags right?

@akoeplinger
Copy link
Member Author

Yup it's 3.20 now: #86530
I'll use the flags instead.

-B "$3"

popd
exit $?
Copy link
Member Author

Choose a reason for hiding this comment

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

still keeping this so it's clearer for the next person touching this line that the exit code is important

@am11
Copy link
Member

am11 commented Nov 29, 2023

Duplicate of #94353?

@akoeplinger
Copy link
Member Author

Thanks, closing this one then.

@akoeplinger akoeplinger deleted the fix-genbuildsys branch November 29, 2023 16:12
ivdiazsa added a commit that referenced this pull request Nov 29, 2023
…em (#94353)

* Successfully replaced pushd/popd with CMake's -S/-B in gen-buildsys.sh
for native and clr. Now, looking into removing other instances if
    possible.

* Removed redundant pushd/popd in src/native/libs/build-native.cmd

* Experimenting with removing 'pushd/popd' for Wasm/Wasi.

* Restored pushd/popd to eng/native/build-commons.sh because it is
actually not directly related to CMake.

* Add explicit exit code to gen-buildsys.sh

Make sure we don't forget to return the cmake exist code, see #95408.

* Replace with comment instead

---------

Co-authored-by: Alexander Köplinger <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants