Fsview extensions by mathstuf · Pull Request #5415 · spack/spack

mathstuf · 2017-09-21T14:57:30Z

This is a rebase of #3227 that should be a lot easier to review. Basically, extension availability for views is now computed based on the view itself rather than looking globally.

Cc: @scheibelp

healther · 2017-09-21T20:45:35Z

We are aware of an usability problem with the current state of #3227 due to the fact that some packages ship their own version of required libraries in the same position as "system" libraries.

One example is llvm which provides its own lib/python2.7/site-packages/six.py which conflicts with the lib/python2.7/site-packages/six.py from py-six. When adding both llvm and py-six to the same view these files collide.
In the current state of #3227 this ends in an exception and the rest of the linking is aborted. As the "list "of packages to add is actually a set, the order of the linking is not guaranteed and we ended up adding an ignore-all-conflicts-by-skipping-existing-packages flag to the spack view add parser. Unfortunately we haven't managed to put that in #3227 yet.

We also thought about implementing some smarter version, i.e. deciding automatically whether to replace, skip or merge two files. However llvm/six.py and py-six/six.py where identical only up to formatting. So here it would have been save to choose either one, but we were uncomfortable with an automatic solution, as the content of the two files where not identical.
So we avoided the problem by just ignoring it and telling the people using it: Keep your eyes open, there might be inconsistent dependencies in this view. So far we haven't had any complaints...

cc @obreitwi

mathstuf · 2017-09-21T22:38:42Z

LLVM should vendor it's six.py install to not conflict. That is a bug with LLVM that should be addressed.

scheibelp · 2017-09-21T23:52:38Z

There appears to be enough support for ignoring conflicts that the view command would only need to add an --ignore-conflicts option; there appears to even be some code which looks for this option. When it's enabled it would print warnings when paths conflict (instead of failing).

healther · 2017-09-25T09:00:37Z

If I recall correctly that only handled the "loading" of packages correctly but not the "activation" of packages

mathstuf · 2017-10-05T19:33:11Z

I've added code which:

fixes spack extensions listing of installed packages: it was just listing activated packages;
the extensions, activate, and deactivate commands now take a --view (or -v) argument to operate on a view; and
spack extensions --show <packages|installed|activated|all> is now implemented.

healther · 2017-10-05T19:54:46Z

lib/spack/spack/database.py

+                    yield spec.package
+            except spack.directory_layout.NoSuchExtensionError:
+                continue
+            # TODO: conditional way to do this instead of catching exceptions


why is catching exceptions a bad idea?

This is copied from the old implementation. I imagine what is wanted is that instead of throwing an exception when a package doesn't extend a given spec is to return False or some other well-typed return value instead.

Ah sorry, I mistook the code position. I actually have no idea where this comes into play. However in general I prefer python code that indicates violated assumptions by exceptions instead of using a special invalid marker of the normal return value. I'm not too sure why the exception is caught in the first place (and especially swallowed without a logged event..)

Looking at the code that does this, it really should return a bool. Currently it throws or it doesn't.

And this code doesn't need the catch, it is check_activated that does the weird control flow.

mathstuf · 2017-10-13T17:51:27Z

I found the bit which needed fixed for testing to work (a new variable added on this PR needed mocked out as well). Seems I need to fix some Python3 issues.

mathstuf · 2017-10-18T18:27:01Z

Fixed the Python3 error. The YAML dumper needed to be told what encoding to use explicitly.

scheibelp · 2017-10-18T22:17:47Z

lib/spack/spack/filesystem_view.py

+
+        set(map(self._check_no_ext_conflicts, extensions))
+        # fail on first error, otherwise link extensions as well
+        if all(map(self.add_standalone, standalones)):


I'm starting to look at this now. I noticed if one of the packages is external and has a prefix like:

packages: openssl: paths: [email protected]: /usr/local

That all of /usr/local will be added. Could you add a check to skip external specs here?

Ew, yeah, I'll do that.

OK, fixed and I've added a test for it.

scheibelp

I have a couple requests:

Expanded testing: I specifically mention in comments what I think would be worthwhile. To be clear my goal is not to meet the percent threshold to get a check
Be more strict when looking for globally-activated packages in view.view

Let me know if these suggestions seem off track.

scheibelp · 2017-10-23T21:00:46Z

lib/spack/spack/cmd/view.py

+        # status and remove can map the name to packages in view
+        specs = relaxed_disambiguate(args.specs, view)
+
+    activated = list(filter(lambda s: s.package.is_extension and


The main goal of collecting these is to avoid a case where an extendee package has globally activated extensions and we add it to the view. I think a stronger check here would be to grab the extendee package and check activated_extensions_for. This would also mean changing the checks in test_view_extension_global_activation.

scheibelp · 2017-10-23T21:15:39Z

lib/spack/spack/test/cmd/view.py

+    install('[email protected]')
+    install('[email protected]')
+    viewpath = str(tmpdir.mkdir('view'))
+    view('symlink', viewpath, '[email protected]')


IMO it is also worth confirming that the files in the extension1 package were created in the view prefix.

scheibelp · 2017-10-23T21:27:29Z

lib/spack/spack/test/cmd/view.py

+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack.main import SpackCommand


Could you add a test for removing extensions from views?

IMO it would also be useful to add a test for a file conflict (two different packages have the same file and are added to a view).

mathstuf · 2017-10-26T18:19:21Z

I fixed the symlinking an extendee with global activations and added the other tests as well.

scheibelp · 2017-10-27T02:55:15Z

All changes appear good. However, I was looking through packages which implement activate and realized that perl still refers to spack.store.layout.extension_map. I'm not sure why it does because Package.activate already handles modifying the extension map, so I think the references can just be removed.

mathstuf · 2017-10-27T17:39:10Z

Python does the same thing; I just coverted it over to using the same argument setup.

scheibelp · 2017-10-27T18:20:29Z

The aspell package is also extendable and implements activate in a way that only works for globals.

This may not matter though if aspell doesn't have a PYTHONPATH equivalent. @hartzell (as the contributor of Spack's aspell package definition) do you happen to know if it does?

mathstuf · 2017-10-27T18:42:01Z

Not sure about aspell, but hunspell supports DICPATH. Looking at aspell source code, I don't see one.

scheibelp · 2017-11-02T20:58:09Z

var/spack/repos/builtin/packages/aspell/package.py

+        extensions_layout = args.get("extensions_layout",
+                                     spack.store.extensions)
+        if extensions_layout is not spack.store.extensions:
+            raise ExtensionError(


Also need to import ExtensionError

scheibelp · 2017-11-02T20:58:27Z

var/spack/repos/builtin/packages/aspell/package.py

    #   - extension.prefix.lib instead of extension.prefix in LinkTree()
    #   - dest_dir instead of self.prefix in tree.(find_conflict|merge)()
    def activate(self, extension, **kwargs):
+        extensions_layout = args.get("extensions_layout",


args should be kwargs

scheibelp · 2017-11-02T20:58:53Z

var/spack/repos/builtin/packages/aspell/package.py

        tree.merge(dest_dir, ignore=ignore)

    def deactivate(self, extension, **kwargs):
+        extensions_layout = args.get("extensions_layout",


args should be kwargs

Aspell compiles in the paths it looks for rather than looking relative to its binary location. There are also no environment variables to redirect it other than changing the path to the configuration file it looks at.

This is used in tests to test real packages without actually building and installing them.

scheibelp · 2017-11-03T00:27:12Z

These test failures do not appear related to the edits so I am closing and reopening to restart tests.

scheibelp · 2017-11-03T01:49:37Z

@obreitwi thanks for your work on this!

@mathstuf thanks for the added work (including testing)!

@tgamblin FYI this is now merged

tgamblin · 2017-11-03T05:44:59Z

Thanks everyone!

obreitwi · 2017-11-03T06:16:14Z

@scheibelp @mathstuf Thanks for getting this done!

Fix a typo in "spack view hardlink" introduced in #5415 ("os.hardlink" does not exist).

haampie · 2022-03-03T13:15:29Z

lib/spack/spack/cmd/extensions.py

+                     for s in spack.store.db.installed_extensions_for(spec)]
+
+        if show_all:
+            print


what was the idea here @mathstuf?

Uhh…probably either stray debugging (unlikely) or an incomplete/untested flag combination.

mathstuf mentioned this pull request Sep 21, 2017

Allow for extensions on a per filesystem view basis #3227

Closed

mathstuf mentioned this pull request Oct 4, 2017

Packages that have the same SHA-1 prefix #5587

Closed

mathstuf force-pushed the fsview_extensions branch from c20a555 to 25c3ede Compare October 5, 2017 19:30

scheibelp mentioned this pull request Oct 5, 2017

package: fix extension query predicate #5600

Merged

mathstuf force-pushed the fsview_extensions branch from 25c3ede to 7309d73 Compare October 5, 2017 19:41

healther reviewed Oct 5, 2017

View reviewed changes

mathstuf force-pushed the fsview_extensions branch 3 times, most recently from 41739ab to 237578d Compare October 13, 2017 15:29

mathstuf force-pushed the fsview_extensions branch 3 times, most recently from 19a0ec0 to 8c3bf45 Compare October 18, 2017 18:26

scheibelp reviewed Oct 18, 2017

View reviewed changes

mathstuf force-pushed the fsview_extensions branch 2 times, most recently from 647d7e8 to ae96766 Compare October 19, 2017 18:50

mathstuf mentioned this pull request Oct 19, 2017

Update view command to create symbolic links when the source itself is a symbolic link. #5823

Closed

scheibelp requested changes Oct 23, 2017

View reviewed changes

mathstuf force-pushed the fsview_extensions branch from ae96766 to 916d65a Compare October 26, 2017 18:18

mathstuf force-pushed the fsview_extensions branch from 00c05d4 to bfcb4a3 Compare November 2, 2017 14:19

scheibelp reviewed Nov 2, 2017

View reviewed changes

mathstuf force-pushed the fsview_extensions branch from bfcb4a3 to 42edd52 Compare November 2, 2017 20:59

mathstuf and others added 4 commits November 2, 2017 17:17

aspell: refuse to work with non-global extensions

0074c0e

Aspell compiles in the paths it looks for rather than looking relative to its binary location. There are also no environment variables to redirect it other than changing the path to the configuration file it looks at.

spec: support a test prefix

f133045

This is used in tests to test real packages without actually building and installing them.

test_activations: test Python's extension logic

85ac123

test_activations: also test perl activations

f4facbb

mathstuf force-pushed the fsview_extensions branch from 42edd52 to f4facbb Compare November 2, 2017 21:17

scheibelp approved these changes Nov 2, 2017

View reviewed changes

scheibelp closed this Nov 3, 2017

scheibelp reopened this Nov 3, 2017

scheibelp merged commit 0f896e9 into spack:develop Nov 3, 2017

tgamblin added the environments label Nov 3, 2017

This was referenced Nov 3, 2017

spack view symlink/hardlink dir py-something should create/update easy-install.pth #1789

Closed

spack view needs to merge easy-install.pth files #3245

Closed

This was referenced Nov 3, 2017

Fix spack view hardlink #6130

Merged

spack view needs to install sitecustomize.py for easy_install.pth to be found #6131

Closed

mathstuf deleted the fsview_extensions branch November 3, 2017 20:04

tgamblin mentioned this pull request Nov 4, 2017

Fix R Activation #4516

Closed

scheibelp pushed a commit that referenced this pull request Nov 7, 2017

Fix spack view hardlink (#6130)

ac3a807

Fix a typo in "spack view hardlink" introduced in #5415 ("os.hardlink" does not exist).

healther mentioned this pull request Nov 14, 2017

spack view fails saying share/info/dir already present #6266

Closed

scheibelp mentioned this pull request Nov 21, 2017

spack extensions broken #2895

Closed

adamjstewart mentioned this pull request Nov 21, 2017

Remove spack extensions from list of known issues #6405

Merged

haampie reviewed Mar 3, 2022

View reviewed changes

Conversation

mathstuf commented Sep 21, 2017

Uh oh!

healther commented Sep 21, 2017

Uh oh!

mathstuf commented Sep 21, 2017

Uh oh!

scheibelp commented Sep 21, 2017

Uh oh!

healther commented Sep 25, 2017

Uh oh!

mathstuf commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathstuf commented Oct 13, 2017

Uh oh!

mathstuf commented Oct 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathstuf commented Oct 26, 2017

Uh oh!

scheibelp commented Oct 27, 2017

Uh oh!

mathstuf commented Oct 27, 2017

Uh oh!

scheibelp commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathstuf commented Oct 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp commented Nov 3, 2017

Uh oh!

scheibelp commented Nov 3, 2017

Uh oh!

tgamblin commented Nov 3, 2017

Uh oh!

obreitwi commented Nov 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

scheibelp commented Oct 27, 2017 •

edited

Loading