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

Archive-level symlink support #182

Merged
merged 4 commits into from Mar 18, 2019

Conversation

Projects
2 participants
@slipher
Copy link
Contributor

slipher commented Mar 15, 2019

No description provided.

@slipher

This comment has been minimized.

Copy link
Contributor Author

slipher commented Mar 15, 2019

Also I'm thinking it was silly to support arbitrary-depth symlinks. Probably better to make the cvar a bool and not support depths greater than 1 (unless @illwieckz found any real examples with depth more than 1).

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Mar 15, 2019

Xonotic 20170401 has 439 first-level (../) symlink and, second-level (../../) symlinks and no third-level (../../../) symlinks.

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Mar 15, 2019

Also, some are absolute symlink (but they don't have leading / as expected in q3 vfs), for example:

dds/wakazachi_reflect.dds -> textures/wakazachi_reflect.dds

Note that his one is in fact a symlink to dds/textures/wakazachi_reflect.dds, the implicit dds/ prefix being already handled by dæmon (see #81) at image loading level, it may work if that's not a recursive symlink and the path is returned to engine, file being seen by zip code or not.

I don't have any idea yet to easily check for recursive symlink and for cross pak symlinks.

Edit, I just said something wrong, that's not an absolute symlink, it's a relative one. I just thought that because of the magic of dirname behavior, the dds/wakazachi_reflect.dds -> textures/wakazachi_reflect.dds will already solve to dds/textures/wakazachi_reflect.dds.

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Mar 15, 2019

So, there is no absolute symlink at all, all the symlink that have a / in their target name without having any leading ../ prefix are relative to dds/.

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Mar 15, 2019

OK, given each pk3 is extracted as a pk3dir, this code lists all cross-pk3 symlinks:

for p in *.pk3dir
do
    (
        cd "${p}"
        find . -type l \
        | while read l
        do
            t="$(readlink "${l}")"
            d="$(dirname "${l}")"
            (
                cd "${d}"
                [ -f "${t}" ] || echo "${t}"
            )
        done
    )
done

hint: there is none 🍭

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Mar 15, 2019

@slipher, that looks good to me, why not printing a message (at least a debug one) on each resolved symlink (with original path and final one, it's not a problem if the two names are on two debug messages lines if implementation does not allow better, the fs code is sequential after all)?

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Mar 15, 2019

I confirm this work.

Thanks to in-pak symlink compatibility layer I was able to simplify and make configurable normal map code, fix parallax one, and fix reflection map leveraging xonotic assets, that's very precious!

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Mar 15, 2019

WAIT, the file system error code just make the loading a map fail entirely if a symlink fails to resolve.

Show resolved Hide resolved src/common/FileSystem.cpp
Show resolved Hide resolved src/common/FileSystem.cpp
Show resolved Hide resolved src/common/FileSystem.cpp Outdated

@illwieckz illwieckz added this to To do in Xonotic via automation Mar 16, 2019

slipher added some commits Mar 15, 2019

Add support for symlinks within zip packages
Symlink support is controlled by a cvar which is disabled by default.
Use of the feature is discouraged; it is intended for testing Xonotic
assets. A symlink may only link to files in the same zip archive. This
change is designed to have minimal impact on performance for non-symlink
files, while good performance for symlinks is mostly a non-goal.

Thanks to illwieckz for some parts of this commit.

@slipher slipher force-pushed the slipher:symlink branch from cf324bf to 60fb25b Mar 16, 2019

@slipher

This comment has been minimized.

Copy link
Contributor Author

slipher commented Mar 16, 2019

I addressed the comments. Also by a depth 2 symlink I mean a symlink that points to a symlink that finally points to an honest file. Are there any depth 2 symlinks like that? (Otherwise arbitrary depth support can be removed.)

@slipher

This comment has been minimized.

Copy link
Contributor Author

slipher commented Mar 16, 2019

OTOH it's not really more complex to implement arbitrary depth, so it's probably not worth checking.

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Mar 17, 2019

I used this code to look for recursive symlinks.

resolve() {
	t="$(readlink "${1}")"
	d="$(dirname "${1}")"
	(
		cd "${d}"
		if [ -h "${t}" ]
		then
			printf ' → %s' "${t}"
			resolve "${t}"
		fi
	)
}

for p in *.pk3dir
do
	(
		cd "${p}"
		find . -type l \
		| while read l
		do
			t="$(readlink "${l}")"
			d="$(dirname "${l}")"
			(
				cd "${d}"
				if [ -h "${t}" ]
				then
					printf "%s → %s" "${l}" "${t}"
					resolve "${t}"
					printf '\n'
				fi
			)
		done
	)
done

There is 38 depth-2 symlinks and no one depth-3 symlinks. All depth-2 symlinks are for player model files, not maps.

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Mar 17, 2019

Great, I tested broken link on shader file, it's correctly handled:

Debug: ...loading 'scripts/map_erbium.shader' 
Warn: [FS] Symlink points to nonexistent file 'scripts/map_erbium.shader' → 'scripts/toto.shader' 
Debug: Failed to open 'scripts/map_erbium.shader' for reading: No such file 
Warn: Couldn't load shader file scripts/map_erbium.shader 

I also tested some legit (if it can be legit) symlink loading, it works:

Debug: looking for extra maps for color map: 'textures/phillipk2x/panels/pk02_panels01b' 
Debug: found extra normal map 'textures/phillipk2x/panels/pk02_panels01b_norm' 
Debug: [FS] Symlink resolved: 'dds/textures/phillipk2x/panels/pk02_panels01b_norm.dds' → 'dds/textures/map_oilrig/pk02_panels01b_dark_norm.dds' 
Debug: found extra specular map 'textures/phillipk2x/panels/pk02_panels01b_gloss' 
Debug: [FS] Symlink resolved: 'dds/textures/phillipk2x/panels/pk02_panels01b_gloss.dds' → 'dds/textures/map_oilrig/pk02_panels01b_dark_gloss.dds' 

It's OK to merge.

@illwieckz

This comment has been minimized.

Copy link
Member

illwieckz commented Mar 17, 2019

@slipher I noticed that you like to rebase and merge the fast forward way, so I let you merge this one.

@slipher slipher merged commit e920d3a into DaemonEngine:master Mar 18, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Xonotic automation moved this from To do to Done Mar 18, 2019

@slipher slipher deleted the slipher:symlink branch Mar 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.