Skip to content

module: Fix various errors during refresh#11084

Merged
alalazo merged 2 commits intospack:developfrom
michaelkuhn:module-refresh-env
Feb 24, 2020
Merged

module: Fix various errors during refresh#11084
alalazo merged 2 commits intospack:developfrom
michaelkuhn:module-refresh-env

Conversation

@michaelkuhn
Copy link
Copy Markdown
Member

@michaelkuhn michaelkuhn commented Apr 2, 2019 •
edited by alalazo
Loading

This change fixes (at least) two problems:

  1. proj uses its stage directory during configure_args. However, configure_options does not set up the stage, leading to OSErrors about the stage directory (see tcl module refresh fails for package proj (installed as dep of gdal) #10649). This change will lead to missing configure_args in such cases but allows regenerating the module file and avoids needlessy setting up fake stages during refresh.
  2. libxml2 uses global variables exported by python. Again, configure_options does not set up the environment appropriately, leading to NameErrors (see module refresh fails for libxml2+python #10716). In addition to ignoring NameErrors, this change will set up the package environment so global variables can be found during refresh.

Fixes #10649, #10716
closes #14361

This was referenced Apr 2, 2019
Closed
Closed
@michaelkuhn
Copy link
Copy Markdown
Member Author

michaelkuhn commented Apr 2, 2019

I fixed the failing tests by changing arch to x86_64 as in the other tests. Not sure if this is the right fix, though.

@alalazo alalazo self-assigned this Apr 4, 2019
@alalazo alalazo added bug Something isn't working modules labels Apr 4, 2019
alalazo
alalazo reviewed Apr 4, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

See comment below.

@alalazo alalazo mentioned this pull request Apr 5, 2019
Merged
6 tasks
@pat-s
Copy link
Copy Markdown
Contributor

pat-s commented Apr 30, 2019

@michaelkuhn How close is this to getting merged? Awaiting the proj fix :)

@michaelkuhn michaelkuhn force-pushed the module-refresh-env branch 2 times, most recently from 4c8569c to 59a0152 Compare May 15, 2019 14:43
@michaelkuhn
Copy link
Copy Markdown
Member Author

michaelkuhn commented May 15, 2019

Sorry for the lack of updates. I have just pushed a reworked change that stores configure arguments in a file and uses this to restore them when refreshing module files.

I hope this is what you had in mind, @alalazo. :-)

@michaelkuhn michaelkuhn force-pushed the module-refresh-env branch from 59a0152 to 7d1b317 Compare May 15, 2019 14:46
@michaelkuhn
Copy link
Copy Markdown
Member Author

michaelkuhn commented Jun 17, 2019

@alalazo ping :-)

@michaelkuhn
Copy link
Copy Markdown
Member Author

michaelkuhn commented Jul 15, 2019

It seems that the proj problem has been fixed elsewhere. libxml2+python still exhibits this problem, though. I have updated the PR to apply to current develop.

This is ready to merge from my side.

@alalazo alalazo closed this Dec 2, 2019
@alalazo alalazo reopened this Dec 2, 2019
@alalazo alalazo mentioned this pull request Jan 3, 2020
Closed
@eugeneswalker eugeneswalker mentioned this pull request Jan 9, 2020
Closed
@eugeneswalker
Copy link
Copy Markdown
Contributor

eugeneswalker commented Jan 9, 2020

@michaelkuhn Do you expect this PR to resolve #14449 ?

alalazo
alalazo approved these changes Feb 5, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

Sorry it took so long to review. This LGTM, I only wonder if we can find better names for args_path.

@michaelkuhn
Copy link
Copy Markdown
Member Author

michaelkuhn commented Feb 5, 2020

Sorry it took so long to review. This LGTM, I only wonder if we can find better names for args_path.

Any preferences? How about configure_args_path (it is called configure_argsfile already, after all).

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Feb 5, 2020

How about configure_args_path

👍

@michaelkuhn
Copy link
Copy Markdown
Member Author

michaelkuhn commented Feb 5, 2020

Rebased and renamed, should be good to go.

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Feb 5, 2020

@michaelkuhn Apologies for being a pain. Can you add some tests to verify the behavior and increase coverage? After that I think this will be ready to merge (don't know why the low coverage was not shown before the rebase 🤔 ).

@michaelkuhn
module: Store configure args during build
9f6cc15 
This change stores packages' configure arguments during build and makes
use of them while refreshing module files. This fixes problems such as:

- libxml2 uses global variables exported by python. configure_options does not
  set up the environment appropriately, leading to NameErrors (see spack#10716).

Fixes spack#10716
@michaelkuhn
test: Add tests for stored configure args
c528e9d
@michaelkuhn
Copy link
Copy Markdown
Member Author

michaelkuhn commented Feb 24, 2020

@michaelkuhn Apologies for being a pain. Can you add some tests to verify the behavior and increase coverage? After that I think this will be ready to merge (don't know why the low coverage was not shown before the rebase ).

This needed another rebase due to the recent distributed build changes. I also finally added some tests. Let me know if anything else is missing.

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Feb 24, 2020

This needed another rebase due to the recent distributed build changes. I also finally added some tests. Let me know if anything else is missing.

I'll test drive it again and merge it if no issues are encountered. Thanks @michaelkuhn !

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Feb 24, 2020

Test driven it. For the still low coverage it seems codecov reports are kind of weird as they show modifications to files not touched by this PR (decrease in log_parse.py?). I'll go on and merge and we can always add further tests later.

@alalazo alalazo merged commit a38eb70 into spack:develop Feb 24, 2020
tgamblin pushed a commit that referenced this pull request Mar 20, 2020
@michaelkuhn @tgamblin
modules: store configure args during build (#11084)
3753424 
This change stores packages' configure arguments during build and makes
use of them while refreshing module files. This fixes problems such as in
#10716.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalazo alalazo alalazo approved these changes

Assignees

@alalazo alalazo

Labels

bug Something isn't working modules

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tcl module refresh fails for package proj (installed as dep of gdal)

5 participants

@michaelkuhn @pat-s @eugeneswalker @alalazo @tgamblin