Hard-link local sources instead of symlinking them. #622

Merged
merged 13 commits into from Jul 7, 2016

Conversation

Projects
None yet
3 participants
Member

kyrofa commented Jul 1, 2016

This PR fixes LP: #1590108 by hard-linking local sources instead of symlinking them. If hard links aren't possible, it copies them instead.

- raise EnvironmentError('Cannot pull to target {!r}'.format(
- self.source_dir))
+ elif os.path.isdir(self.source_dir):
+ shutil.rmtree(self.source_dir)
@kyrofa

kyrofa Jul 1, 2016

Member

Take particular note here: previously the code was very careful not to overwrite an existing parts/<name>/src directory or file. That logic is essentially impossible now that we're not symlinking, so I decided to assert ownership over parts/<name>/ and just blow it away if it already exists. See any downside to that? Users shouldn't be putting things there...

@sergiusens

sergiusens Jul 1, 2016

Collaborator

ownership was always the case.
We do need to track source changes now though ;-)

@kyrofa

kyrofa Jul 1, 2016

Member

We do need to track source changes now though ;-)

I'm not sure what you mean. Are you saying something needs to happen here?

@sergiusens

sergiusens Jul 1, 2016

Collaborator

El 01/07/16 a las 18:25, Kyle Fazzari escribió:

In snapcraft/internal/sources.py
#622 (comment):

         os.remove(self.source_dir)
  •    elif (os.path.isdir(self.source_dir) and
    
  •          not os.listdir(self.source_dir)):
    
  •        os.rmdir(self.source_dir)
    
  •    elif os.path.exists(self.source_dir):
    
  •        raise EnvironmentError('Cannot pull to target {!r}'.format(
    
  •            self.source_dir))
    
  •    elif os.path.isdir(self.source_dir):
    
  •        shutil.rmtree(self.source_dir)
    
We do need to track source changes now though ;-)

I'm not sure what you mean. Are you saying something needs to happen here?

Eventually, when we implement tracking of changes to sources that would
affect build results ;)

@kyrofa

kyrofa Jul 1, 2016

Member

Ah, yes of course. That will be a wonderful day! This change brings it a step closer 😃 .

@@ -144,6 +144,9 @@ def build(self):
if os.path.exists(self.build_basedir):
shutil.rmtree(self.build_basedir)
+ # FIXME: It's not necessary to ignore here anymore since it's now done
@sergiusens

sergiusens Jul 1, 2016

Collaborator

please recreate an integration test with this scenario to confirm this

@kyrofa

kyrofa Jul 1, 2016

Member

Done.

snapcraft/tests/test_sources.py
+ os.makedirs(os.path.join('src', 'prime'))
+
+ # Make the snapcraft.yaml and a built snap
+ open(os.path.join('src', 'snapcraft.yaml'), 'w').close()
@sergiusens

sergiusens Jul 1, 2016

Collaborator

ah, we might be missing .snapcraft.yaml here!

@kyrofa

kyrofa Jul 1, 2016

Member

Ah, indeed we are! I'll go ahead and fix that here.

Collaborator

sergiusens commented Jul 1, 2016

looks good, I am reaching EOD so will let @elopio pay attention to the detail in UTs ;-)

kyrofa added some commits Jun 27, 2016

Hard-link local sources instead of symlinking them.
If hard links aren't possible, copy them instead.

LP: #1590108

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Make store download integration test check in part src.
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Add integration test for running in old tree.
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Add .snapcraft.yaml to common.SNAPCRAFT_FILES.
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Fix simple-circle integration test.
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Make old-part-src integration test snap symlink relative.
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Collaborator

sergiusens commented Jul 5, 2016

parts/<part-name>/bin/pkg-config should be erased from existence 😉

Collaborator

sergiusens commented Jul 5, 2016

just that last comment from me :-)

Remove old-part-src pkg-config.
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Member

kyrofa commented Jul 5, 2016

parts/<part-name>/bin/pkg-config should be erased from existence

Done 😉 .

Member

kyrofa commented Jul 6, 2016

Huh... now that I merged in master suddenly the coverage is decreasing instead of increasing. Not sure what the deal is there. Also, even VPNd in I can't seem to get to the autopkgtest failure, though judging from previous results it's just ROS.

Member

kyrofa commented Jul 6, 2016

Huh. Now coverage increased again. Coveralls I think you're full of it.

Member

kyrofa commented Jul 6, 2016

Failing ROS:

ValueError: Invalid placeholder in string: line 1, col 123
\x1b[31m\x1b[1m<==\x1b[0m Failed to process package \'\x1b[1m\x1b[34mtalker\x1b[0m\': 
  Invalid placeholder in string: line 1, col 123\x1b[0m
Command failed, exiting.
\x1b[0;31mCommand \'[\'/bin/sh\', \'/tmp/tmpm099_p1z\', \'/bin/bash\', \'/tmp/tmpzbdd1lf3\']\' returned non-zero exit status 1\x1b[0m
'}}}

Not really sure what the deal is with autopkgtests...

Member

elopio commented Jul 7, 2016

This is all green. The examples error is in the cleanup, fixed here: ubuntu-core/snappy-jenkins#193

@sergiusens sergiusens merged commit a07fa0f into snapcore:master Jul 7, 2016

3 of 4 checks passed

Examples tests
Details
autopkgtest Success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 96.288%
Details

@kyrofa kyrofa deleted the kyrofa:feature/1590108/hardlink_src branch Jul 7, 2016

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

Hard-link local sources instead of symlinking them. (#622)
If hard links aren't possible, copy them instead.

LP: #1590108

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment