Skip to content

Declare scriptroot before its usage#3076

Merged
chcosta merged 1 commit intodotnet:masterfrom
am11:add-missing-var-declaration
Jun 20, 2019
Merged

Declare scriptroot before its usage#3076
chcosta merged 1 commit intodotnet:masterfrom
am11:add-missing-var-declaration

Conversation

@am11
Copy link
Member

@am11 am11 commented Jun 18, 2019

Following error is reported (and ignored) when building CoreCLR:

...
...
Checking prerequisites...
/datadrive/projects/coreclr/eng/common/tools.sh: line 359: /pipeline-logging-functions.sh: No such file or directory
/datadrive/projects/coreclr/eng/common/tools.sh: line 359: /pipeline-logging-functions.sh: No such file or directory
Laying out dynamically generated EventSource classes
...
...

@am11
Copy link
Member Author

am11 commented Jun 19, 2019

@jkotas, is this the correct repository for this change? I backtraced it to this location as origin. :)

@filipnavara
Copy link
Member

filipnavara commented Jun 19, 2019

I am hitting this issue with zsh on macOS Catalina. Thanks for fixing it.

@jkotas jkotas requested a review from chcosta June 19, 2019 13:08
@jkotas
Copy link
Member

jkotas commented Jun 19, 2019

@chcosta Looks like this issue was introduced by your recent change.

chcosta
chcosta approved these changes Jun 19, 2019
View reviewed changes
Copy link
Member

@chcosta chcosta left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this!

@chcosta chcosta mentioned this pull request Jun 19, 2019
Closed
chcosta
chcosta requested changes Jun 19, 2019
View reviewed changes
Copy link
Member

@chcosta chcosta left a comment

Choose a reason for hiding this comment

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

I feel like this variable ("script_root") is pervasive enough that it should be moved into tools.sh (as you've done here), but also removed from the other scripts in eng\common which define / use the variable and import tools.sh. Also, to reverse on my previous statement, it's possible that some repo now has a dependency on the name "scriptroot" and we should probably stick with that to be less disruptive.

@chcosta
Copy link
Member

chcosta commented Jun 20, 2019

I had some time to look into the reported issue further.

I don't think the pipeline-logging-functions.sh file is the reason this build failed

https://dev.azure.com/dnceng/public/_build/results?buildId=228032&view=logs&jobId=52330f83-6c02-575b-9cc9-003381b2b65d&taskId=ae1f33b6-fe71-5768-c1cf-5c9519336510&lineStart=3908&lineEnd=3913&colStart=2&colEnd=2

It looks like the build progresses past that point and fails for other reasons. "scriptroot" is not set because in standard Arcade repos, that variable is set by the calling scripts in every case. CoreClr (and other repos like CoreClr, e.g. Roslyn) which do some custom things, don't necessarily set this variable.

I think the simplest fix here is to just move the declaration down a couple of lines and change, https://github.com/dotnet/arcade/blob/master/eng/common/tools.sh#L359-L362

to

ResolvePath "${BASH_SOURCE[0]}"
_script_dir=`dirname "$_ResolvePath"`

. "$_script_dir/pipeline-logging-functions.sh"

@am11
Copy link
Member Author

am11 commented Jun 20, 2019

There are multiple (10+) scripts in this repo, where scriptroot is resolved as done in
53c780f commit: https://github.com/dotnet/arcade/search?q=scriptroot.
Are there plans to convert all of them to use ResolvePath "${BASH_SOURCE[0]}"?

@am11
Declare scriptroot before its usage
172b910 
Following error is reported (and ignored) when building CoreCLR:

```sh
...
...
Checking prerequisites...
/datadrive/projects/coreclr/eng/common/tools.sh: line 359: /pipeline-logging-functions.sh: No such file or directory
/datadrive/projects/coreclr/eng/common/tools.sh: line 359: /pipeline-logging-functions.sh: No such file or directory
Laying out dynamically generated EventSource classes
...
...
```
@agocke agocke requested a review from chcosta June 20, 2019 20:01
@chcosta chcosta merged commit cfad79c into dotnet:master Jun 20, 2019
@am11 am11 deleted the add-missing-var-declaration branch June 21, 2019 07:35
@agocke agocke mentioned this pull request Jun 21, 2019
Merged
agocke added a commit to dotnet/roslyn that referenced this pull request Jun 21, 2019
@agocke
Bring changes from dotnet/arcade#3076
11ec096
tmat pushed a commit to dotnet/roslyn that referenced this pull request Jun 24, 2019
@dotnet-maestro @tmat
[master] Update dependencies from dotnet/arcade (#36651)
d3243bc 
* Update dependencies from https://github.com/dotnet/arcade build 20190620.1

- Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19320.1

* Bring changes from dotnet/arcade#3076

* Update dependencies from https://github.com/dotnet/arcade build 20190621.75

- Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19321.75

* Update dependencies from https://github.com/dotnet/arcade build 20190622.2

- Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19322.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agocke agocke agocke approved these changes

@jkotas jkotas jkotas approved these changes

@chcosta chcosta chcosta approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@am11 @filipnavara @jkotas @chcosta @agocke