Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

add option to specify clang and few minor improvements for Linux#23302

Merged
wfurt merged 2 commits into
dotnet:masterfrom
wfurt:clang
Aug 17, 2017
Merged

add option to specify clang and few minor improvements for Linux#23302
wfurt merged 2 commits into
dotnet:masterfrom
wfurt:clang

Conversation

@wfurt
Copy link
Copy Markdown
Member

@wfurt wfurt commented Aug 16, 2017

fixes #23299 - build on Ubuntu 17.04

build-native.sh under src/Native already has option to specify clang but that is not exposed to top level and build.sh clang3.8 fails with complain about unknown option.

The options are really inconsistent, some starting with --XX, some -YY and some ZZZ.
To keep it consistent wit top level options and help I added -Clang=XYZ so one can pass any string without need to update script for every new clang version (which builds-managed.sh clangx.y does)

There are also three more minor improvements:

  1. -h works now same way as -? (it always did for src/Native/build-native.sh)
    • we do not try to use options starting with '-' as directory. Preferably the check -d $1 should skip all options like normal scripts do or there should be explicit option for that.
  2. we use "$@" instead of $* to properly handle arguments with spaces. I did this while back but it got rolled back because it exposed some other problem (which is fixed now)

I did test build on Ubuntu 17.04
./build.sh -Clang=clan3.8

I also did basic testing on OSX.

./build.sh -h

...
[-ProducesTarget] MsBuild target that displays all of the artifacts this repo produces.

[-DirectoryToBuild] MsBuild property used to set the directory to scope the build to things under that
directory.
=> Default value: Please-Specify-A-Directory

[-Clang] Specific version of Clang to use e.g. clang3.7, clang3.8, etc ...

[-RunQuiet] Run tool specific setting. Set to True to only display output from the executing
command.
=> Default value: false
=> Legal values: [true, false].

this should fix panda20 build problem @geoffkizer

@wfurt wfurt self-assigned this Aug 16, 2017
@dnfclas
Copy link
Copy Markdown

dnfclas commented Aug 16, 2017

@wfurt,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

Copy link
Copy Markdown
Member

@MattGal MattGal left a comment

Choose a reason for hiding this comment

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

While these changes look benign to me, so did the previous ones which broke the build. The best thing to do would be to grab the logs from the same flavor of official build that failed last time you did this and run through the exact commands from such a build to double check things work.

Comment thread build-managed.sh
@@ -1,4 +1,4 @@
#!/usr/bin/env bash
working_tree_root="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
$working_tree_root/run.sh build-managed $*
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm by no means an expert in shell scripting so could you please explain the difference between $* and "$@"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've been talking about this @MattGal
The difference is is you pass in something like "aaa bb" The existing form would pass it as two parameters to calling script, "$@" preserves it as single parameter. I bumped into this while back when I was trying to pass multiple options to xunit

https://stackoverflow.com/questions/12314451/accessing-bash-command-line-args-vs/12316565

I'm going to roll back this and submit that as separate PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I've run into this issue as well. This works on Windows but is broken on Linux:

msbuild.sh /t:RebuildAndTest /p:XunitOptions="-showprogress -parallel none"

Copy link
Copy Markdown
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

What is the plan for getting the builds to automatically work on supported platforms without needing to pass in the version?

@wfurt
Copy link
Copy Markdown
Member Author

wfurt commented Aug 17, 2017

I don't know @weshaggard . I would be happy to help with *NIX build improvements but perhaps we can sync up on priority list.
Do we want the dotnet project to be as similar as possible? As I mentioned some of this already exist but there is difference between coreclr and corefx (and perhaps other repos)
I also have some build changes to support FreeBSD (not completed) but I don't know what would be best strategy to get them in.

@weshaggard
Copy link
Copy Markdown
Member

@dleeapho looks like this is another piece of the toolset acquisition problem.

@wfurt wfurt merged commit 39b4505 into dotnet:master Aug 17, 2017
@wfurt wfurt deleted the clang branch August 17, 2017 22:38
@karelz karelz modified the milestone: 2.1.0 Aug 20, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…net/corefx#23302)

* add option to specify clang and few minor improvements

* roll-back $* $@ change. that should be separate PR


Commit migrated from dotnet/corefx@39b4505
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.

corefx does not build on Ubuntu 17.04

6 participants