Skip to content

LibraryList and HeaderList not ordered properly#4156

Merged
tgamblin merged 1 commit intospack:developfrom
epfl-scitas:fixes/order_of_files_may_be_incorrect
May 11, 2017
Merged

LibraryList and HeaderList not ordered properly#4156
tgamblin merged 1 commit intospack:developfrom
epfl-scitas:fixes/order_of_files_may_be_incorrect

Conversation

@alalazo
Copy link
Copy Markdown
Member

@alalazo alalazo commented May 7, 2017

PR #3367 inadvertently changed the semantics of _find_recursive and _find_non_recursive so that the returned list are not ordered as the input search list. This commit restores the original semantic, and adds tests to verify it.

@alalazo alalazo added bug Something isn't working ready labels May 7, 2017
@alalazo alalazo requested review from adamjstewart and tgamblin May 7, 2017 09:58
@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented May 7, 2017

Forgot to mention: this is a real issue when building, for instance, any Makefile based package against Intel MPI or MKL as you'll get the library order scrambled.

Copy link
Copy Markdown
Member

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

I guess I'm not sure of the performance impact of poking the filesystem. Is the more complicated code worth the time savings?

found_files = []

# The variable here is **on purpose** a defaultdict. The idea is that
# we want to poke the filesystem as least as possible, but still maintain
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.

as least -> as little

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.

Fix incoming, pretend I never did this.

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented May 7, 2017

I guess I'm not sure of the performance impact of poking the filesystem. Is the more complicated code worth the time savings?

So, two things:

  1. The code as it was before was not inefficient, it was buggy
  2. I could have swapped two for loops in both functions, but then I would have traversed root and descendants a number of time equal to len(search_files), and decided not to do it because just one time was necessary.

… output)

PR spack#3367 inadvertently changed the semantics of _find_recursive and
_find_non_recursive so that the returned list are not ordered as the
input search list. This commit restores the original semantic, and adds
tests to verify it.
@alalazo alalazo force-pushed the fixes/order_of_files_may_be_incorrect branch from 6fa116c to 6fae442 Compare May 7, 2017 15:36
@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented May 11, 2017

@tgamblin ping

@tgamblin tgamblin merged commit f8b3eff into spack:develop May 11, 2017
@alalazo alalazo deleted the fixes/order_of_files_may_be_incorrect branch May 11, 2017 17:46
diaena pushed a commit to diaena/spack that referenced this pull request May 26, 2017
… output) (spack#4156)

PR spack#3367 inadvertently changed the semantics of _find_recursive and
_find_non_recursive so that the returned list are not ordered as the
input search list. This commit restores the original semantic, and adds
tests to verify it.
xavierandrade pushed a commit to xavierandrade/spack that referenced this pull request Jun 16, 2017
… output) (spack#4156)

PR spack#3367 inadvertently changed the semantics of _find_recursive and
_find_non_recursive so that the returned list are not ordered as the
input search list. This commit restores the original semantic, and adds
tests to verify it.
EmreAtes pushed a commit to EmreAtes/spack that referenced this pull request Jul 10, 2017
… output) (spack#4156)

PR spack#3367 inadvertently changed the semantics of _find_recursive and
_find_non_recursive so that the returned list are not ordered as the
input search list. This commit restores the original semantic, and adds
tests to verify it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants