Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dependency injection and shared cache #32

Merged
merged 1 commit into from
Jan 5, 2018

Conversation

HebaruSan
Copy link
Member

Background

KSP-CKAN/CKAN-meta#1316 tried to update the download links for four mods, two of which are dependencies for the other two. The pull request validation failed, and then it got interesting.

Problem 1: Metadata injection

The URL that failed to download should not exist anymore, because it's overwritten as part of that pull request:

https://ci.ksp-ckan.org/job/CKAN-meta/886/consoleFull

Failed to download "https://github.com/SamBelanger/SamBelangerFlags/releases/download/v1.0/SamBelangerFlags.zip" - error: The remote server returned an error: (404) Not Found.
One or more files failed to download, stopped.
Build step 'Execute shell' marked build as failure

Cause

This function is supposed to take care of that by replacing old metadata with the latest copy from the pull request:

# ------------------------------------------------
# Function for injecting metadata into a tar.gz
# archive. Assummes metadata.tar.gz to be present.
# ------------------------------------------------
inject_metadata () {

However, the line that does it copies the new metadata files to the root of the CKAN-meta-master directory:

for f in ${OTHER_FILES[*]}
do
echo "Injecting $f"
cp $f CKAN-meta-master
done

The old metadata is not in the root; it's in identifier/identifier-version.ckan. This means that the old metadata is not overwritten, and instead both the old and new metadata will be in the local repo. Which one will actually be used by ckan? Luck of the draw.

Changes

Now we copy the new metadata to the exact location it will occupy once it is merged. This ensures that it will overwrite any files it is replacing, so old metadata will not be used for installation and testing. Both CKAN-meta and NetKAN are updated.

Problem 2: Shared download cache

In the process of investigating that, I noticed another issue. This line does not work as intended:

# Link to the downloads cache.
ln -s ../../downloads_cache/ dummy_ksp/CKAN/downloads/

This is supposed to make all instances of ckan use the same download directory, to save bandwidth and time. However, currently it doesn't work; for example, the script downloaded GPP 1.5.88 (and its dependencies) three times for this older pull request:

https://ci.ksp-ckan.org/job/CKAN-meta/880/consoleFull

They should have shown up as "(cached)" in download listings after the first and not been redownloaded. At nearly 300 MB per cycle, that's a lot of downloading.

Cause

What happens is hiding in plain sight in that log:

No override, Running with '1.3.1'
Creating a dummy KSP '1.3.1' install
Creating /home/jenkins/workspace/CKAN-meta/dummy_ksp/CKAN/downloads
Added "a87cdf2483a36d18d8cef50e54b7d068db23a50d" with root "/home/jenkins/workspace/CKAN-meta/dummy_ksp" to known installs

Before the ln command runs, ckan.exe adds the dummy instance to the list of known instances, and in the process it initializes the instance's CKAN folder, including creating the downloads folder.

The ln command runs later, and its target folder already exists. When a ln command is given a destination folder argument that already exists, it creates a new sym link inside that folder; so what's happening is that we now have a symlink here:

/home/jenkins/workspace/CKAN-meta/dummy_ksp/CKAN/downloads/downloads_cache/

... which doesn't get used by ckan.exe. Instead the normal non-shared downloads folder is used and wiped between tests.

Changes

Now we create the download folder symlink before ckan.exe runs. This will ensure that it is shared across all tests for the same commit. Both CKAN-meta and NetKAN are updated.

Command verbosity

To make it easier to spot problems like these in the future, the --verbose flag is added to all standard Unix commands used in the scripts, such as rm, mkdir, cp, etc. This generates an extra line of output per command that spells out the action being taken:

$ cp --verbose test1 test2
'test1' -> 'test2'

This way we will be able to tell exactly which paths are being used, and any funny business will be more obvious.

@HebaruSan HebaruSan added the Bug label Jan 4, 2018
@techman83
Copy link
Member

Fantastic work @HebaruSan!

@techman83 techman83 merged commit 745d6de into KSP-CKAN:master Jan 5, 2018
@HebaruSan HebaruSan deleted the fix/injection-and-cache branch January 5, 2018 00:26
@techman83
Copy link
Member

I did find a bug though, I'll take a look see 🙂

Creating a dummy KSP '1.3.1' install
mkdir: created directory 'dummy_ksp'
mkdir: created directory 'dummy_ksp/CKAN'
mkdir: created directory 'dummy_ksp/GameData'
mkdir: created directory 'dummy_ksp/Ships/'
mkdir: created directory 'dummy_ksp/Ships/VAB'
mkdir: created directory 'dummy_ksp/Ships/SPH'
mkdir: created directory 'dummy_ksp/Ships/@thumbs'
mkdir: created directory 'dummy_ksp/Ships/@thumbs/VAB'
mkdir: created directory 'dummy_ksp/Ships/@thumbs/SPH'
ln: target 'dummy_ksp/CKAN/downloads/' is not a directory: No such file or directory

@techman83
Copy link
Member

Aha, trailing slash.

➜  /tmp ln -s --verbose ../../downloads_cache dummy_ksp/CKAN/downloads/
ln: target 'dummy_ksp/CKAN/downloads/' is not a directory: No such file or directory 

vs

➜  /tmp ln -s --verbose ../../downloads_cache dummy_ksp/CKAN/downloads
'dummy_ksp/CKAN/downloads' -> '../../downloads_cache'

@techman83
Copy link
Member

I'll have a PR in a moment @HebaruSan

@techman83
Copy link
Member

Actually, I need more coffee. No trailing slash in the file!

@techman83
Copy link
Member

Ohhh you beat me to it, thanks 😄

@HebaruSan
Copy link
Member Author

Yeah, I'm treating the original CKAN-meta PR as a test case. I also took out the verbose flag for two of the recursive rm commands, since they are deleting whole directory trees and listing out every single file, which is just noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants