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

Make FPU settings configurable for installer #32053

Merged
merged 1 commit into from
Feb 11, 2020
Merged

Make FPU settings configurable for installer #32053

merged 1 commit into from
Feb 11, 2020

Conversation

am11
Copy link
Member

@am11 am11 commented Feb 10, 2020

#32029

@jkotas, seems like CLR_ARM_FPU_CAPABILITY change in eng/common/toolchain.cmake was overwritten by @dotnet-bot at 4528f84#diff-ee3a7f0d86bfe417f1acbec7d1c0082a. I think the proper way was to push the changes https://github.com/dotnet/arcade/.

eng/common/toolchain.cmake is only used in one place in the runtime repo, so I have copied it to eng/native/ directory, next to where it is used. We can delete this file from dotnet/arcade.

@jkotas
Copy link
Member

jkotas commented Feb 10, 2020

We can delete this file from dotnet/arcade.

Could you please submit PR to delete it ? There are 3 toolchain.cmake in different directories - should all of them be deleted?

@am11
Copy link
Member Author

am11 commented Feb 10, 2020

Aside, I have ran cmake-format on the copied toolchain.cmake file, using this configuration:

# file: $HOME/cmake-format.config

# -----------------------------
# Options effecting formatting.
# -----------------------------
with section("format"):

  # How wide to allow formatted cmake files
  line_width = 180

  # How many spaces to tab for indent
  tab_size = 4

  # If true, separate flow control names from their parentheses with a space
  separate_ctrl_name_with_space = True

  # If true, separate function names from parentheses with a space
  separate_fn_name_with_space = True

  # If a statement is wrapped to more than one line, than dangle the closing
  # parenthesis on its own line.
  dangle_parens = False

and ran:

pip install cmake_format

cd dotnet-runtime
$HOME/.local/bin/cmake-format -i -c=$HOME/cmake-format.config eng/native/toolchain.cmake

it would be nice if we run it on all *.cmake and **/CMakeLists.txt files at some point and checked in from @dotnet-bot account:

git ls-files :/**/*.cmake :/**/CMakeLists.txt | \
    xargs --null -I{} $HOME/.local/bin/cmake-format -i -c=$HOME/cmake-format.config {}

there are over 1532 cmake files in the repo.

@am11
Copy link
Member Author

am11 commented Feb 10, 2020

Could you please submit PR to delete it ?

Sure. @janvorli, shall we move eng/common/cross to eng/native/cross and delete eng/common/toolchain.cmake and eng/common/cross/ from dotnet/arcade? Seems like dotnet/runtime repo is the only current consumer of those parts of arcade.

@janvorli
Copy link
Member

janvorli commented Feb 10, 2020

Why do we want to move them out of the arcade? In my view, it belongs to the cross build that can be used by any repo, not just runtime. I would expect corert to use it too. So I would keep it in the arcade.

@janvorli
Copy link
Member

Edited the above message: runtime -> arcade

@janvorli
Copy link
Member

@jkotas the three versions are: one for Linux, two for Android (arm, arm64). I guess the Android ones could be at least merged together if not into the Linux one. But I haven't looked at them for quite a long time, so I am not sure whether there are any caveats or not.

@janvorli
Copy link
Member

@am11 I personally don't like the separate_ctrl_name_with_space = True for everything. While it seems acceptable for conditionals for me, I really don't like it for other stuff like include_directories, message, etc. So maybe unifying it the other way - no space (which seems to be prevalent in our CMakeLists.txt) would be a preferable way.

@am11
Copy link
Member Author

am11 commented Feb 10, 2020

@janvorli, sure, noted. I have reverted the change in eng/ dir and upstream it in dotnet/arcade repo (without formatting). We can do the formatting change separately (perhaps after #31701 is done).

@am11 am11 changed the title Move toolchain.cmake to eng/native/ Make FPU settings configurable for installer Feb 10, 2020
@am11
Copy link
Member Author

am11 commented Feb 11, 2020

@janvorli, is this change ok to check in? Once Arcade is updated, original issue can be closed.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli janvorli merged commit 5b2eadc into dotnet:master Feb 11, 2020
@am11 am11 deleted the fix/cross-toolchain branch November 29, 2020 12:08
@ghost ghost locked as resolved and limited conversation to collaborators Dec 29, 2020
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