Fix environment variable handling #229

Merged
merged 1 commit into from Jan 15, 2016

Conversation

Projects
None yet
3 participants
Collaborator

sergiusens commented Jan 13, 2016

Environment variables now correctly point to stagedir and snapdir and
don't expose the internal installdir paths for a part.

This required an intermediate change in the handling of stage-packages
but it also gives it a semantic meaning by unpacking everything into
the staging directory. Future work can make the handling of
stage-package more transactional or in a single shot.

LP: #1531481

Collaborator

sergiusens commented Jan 13, 2016

This can probably be refactored some more to not need build_env_for_part

+ 'foo/usr/lib/x86_64-linux-gnu/pkgconfig:foo/usr/share/pkgconfig:'
+ 'foo/usr/local/lib/pkgconfig:'
+ 'foo/usr/local/lib/x86_64-linux-gnu/pkgconfig:'
+ 'foo/usr/local/share/pkgconfig:$PKG_CONFIG_PATH' in environment)
@kyrofa

kyrofa Jan 13, 2016

Member

Are you actually wanting to test the ordering here, or do you only really only want to test that each of these are included in PKG_CONFIG_PATH? If you're not wanting to test the order, this test is perhaps overly fragile. That goes for most of these assertions.

@sergiusens

sergiusens Jan 13, 2016

Collaborator

Well I also want to test that the output is correct and no missing colon gets in there. The order is fixed though (and should be as it determines the search order too).

@kyrofa

kyrofa Jan 13, 2016

Member

Well I also want to test that the output is correct and no missing colon gets in there.

You can do this without testing the order of them.

The order is fixed though (and should be as it determines the search order too).

Good enough for me. Just wanted to make sure.

Member

kyrofa commented Jan 13, 2016

Yeah this looks good to me!

+ patcher = unittest.mock.patch('snapcraft.common.get_arch_triplet')
+ mock_arch = patcher.start()
+ mock_arch.return_value = 'x86_64-linux-gnu'
+ self.addCleanup(patcher.stop)
Collaborator

sergiusens commented Jan 14, 2016

I am at a point where the only thing failing is ROS. Let's see how this goes.

Fix environment variable handling
Environment variables now correctly point to stagedir and snapdir and
don't expose the internal installdir paths for a part.

This required an intermediate change in the handling of stage-packages
but it also gives it a semantic meaning by unpacking everything into
the staging directory. Future work can make the handling of
stage-package more transactional or in a single shot.

LP: #1531481
Collaborator

sergiusens commented Jan 14, 2016

Everything builds now; most things built work too

Member

elopio commented Jan 14, 2016

👍
I see on the bug report that you requested an example. That would be awesome. Should we wait for it?

Collaborator

sergiusens commented Jan 15, 2016

I did a rebase of these onto the catkin one and it all works great.

sergiusens added a commit that referenced this pull request Jan 15, 2016

Merge pull request #229 from sergiusens/env-fixes
Fix environment variable handling

@sergiusens sergiusens merged commit 90075d0 into snapcore:master Jan 15, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 90.753%
Details

@sergiusens sergiusens deleted the sergiusens:env-fixes branch Jan 15, 2016

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

Merge pull request #229 from sergiusens/env-fixes
Fix environment variable handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment