[minor] Non-silenced pushd followed by popd in tensorflow/configure

[z-a-f]

tensorflow/configure has the following lines:

#!/usr/bin/env bash

set -e
set -o pipefail

# Find out the absolute path to where ./configure resides
pushd `dirname $0` #> /dev/null
SOURCE_BASE_DIR=`pwd -P`
popd > /dev/null

I am not sure what is the purpose of the pushd followed by popd without change in directory. I think this is not necessary, and distracts when reading the source codes. Unless it is allowed to run the configure from the directory other than the tensorflow root, it is completely redundant.

Also, the > /dev/null is commented out which causes the current directory to print when running the script:

$ ./configure
  ~/GitHub/tensorflow ~/GitHub/tensorflow
  Please specify the location of python. [Default is ~/.virtualenvs/tf-dev-env/bin/python]:
  .........

[gunan]

@aselle looks like you added the lines in question?

I believe this would be a sanity check, just making sure we are at the root.

[aselle]

done

…

On Wed, Dec 28, 2016 at 11:20 PM gunan ***@***.***> wrote: @aselle <https://github.com/aselle> looks like you added the lines in question? I believe this would be a sanity check, just making sure we are at the root. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6555 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAT52uw5qZBa5nQq04cAnDuRb_9K6rWhks5rM19IgaJpZM4LXZRG> .

[z-a-f]

The pushd is silenced, but do we actually need it? I can see the use for it if we allow users to run configure from custom defined source directories. However, even in that case I would probably use something like

SOURCE_BASE_DIR=`pwd -P`
pushd ${PWD} >/dev/null
cd `dirname $0`
#
# Perform some actions in the directory where 'configure' is
#
popd >/dev/null
#
# Perform some actions in the directory where the scipt was called from
#

[aselle]

I'm confused. That code does seems to have an extra line and capture SOURCE_BASE_DIR of where you run it from rather than where configure is.

[z-a-f]

@aselle That what the original script was doing -- I assumed it was on purpose -- the only reason it would be required is if there are two different locations that the script would have to work with: one where SOURCE_BASE_DIR is, and another where the configure is located. Assuming TF allows them to be in different locations, it makes sense to keep one of them on a stack, while working in the other. Otherwise, I think the script should be

#!/usr/bin/env bash

set -e
set -o pipefail

SOURCE_BASE_DIR=`pwd -P`

I was trying to see the Blame for the popd, pushd lines, and this is the only thing there:

It is necessary to symlink in files from .git/ in order to make
bazel aware of changes to the current head. As it is this is not
completely reliable when git repositories are in a dirty index
state. First class support for bazel git a reported bug but
not a high priority.

./configure sets up the symlinks by calling the gen_git_source.py
a bazel genrule calls gen_git_source.py to generate version_info.cc

Also changed cmake and make to build this properly.
Change: 132328009

[gunan]

@zafartahirov I think for using tensorflow as is, you have a point. However, when tensorflow is an external dependency, such as how tensorflow/models or tensorflow/serving use TF, I believe users may need to run configure differently.

Also, as the pushd/popd lines have a comment describing what they do, I do not see a problem in terms of code readability.

Therefore, I am inclined to close this issue. Let me know if the above explanations do not address your concerns. We can reopen the issue if you disagree with my assessment above.