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

disable repository search in parent directories #675

Merged
merged 4 commits into from Feb 9, 2023

Conversation

pagran
Copy link
Member

@pagran pagran commented Feb 8, 2023

git is very smart and tries to find repository in parent directory.
This is a very strange situation, but in our modinfo test, this is exactly what happens:

toor@root:/tmp/go-test-script3218627159/script-modinfo/.tmp/garble-shared3668927587/linker-src$ git log
commit 36f59e140927694cf85e88e1d4bf7440474f89c7 (HEAD -> master)
Author: name <name@email.local>
Date:   Wed Feb 8 00:38:26 2023 +0100

    very unique commit message

Fix prevents this behavior

@pagran pagran requested a review from mvdan February 8, 2023 09:39
@mvdan
Copy link
Member

mvdan commented Feb 8, 2023

Please document in the code why we need to set the flag on top of the Dir field.

Also, are we actually sure that this will fix the bug? We already tried once and didn't get it :)

I first thought that writing a regression test would be good, to ensure the code works, but then again we'll stop using git apply hopefully sooner than later. So I don't think it's needed.

@pagran
Copy link
Member Author

pagran commented Feb 8, 2023

I can extend modinfo test with linker rebuild and additionally add a panic in case of a "silent" patch skip

@mvdan
Copy link
Member

mvdan commented Feb 8, 2023

I don't think it's worth it to be honest. The test will be slow, it has nothing to do with module version info, and we'll stop relying on executing git soon. But we should manually verify that the bug is fixed.

@pagran
Copy link
Member Author

pagran commented Feb 8, 2023

But it makes sense to add an extra check in case of "silent" skipping patches

@mvdan
Copy link
Member

mvdan commented Feb 8, 2023

Sure, why not. Perhaps you can do something like:

[!short] env OUR_LINKER_CACHE=/some/empty/dir

...

# ensure no panic
[!short] exec built-binary

You should verify this with master, but I think this would always trigger the failure, and it would keep go test -short as fast as before.

@mvdan
Copy link
Member

mvdan commented Feb 8, 2023

Or, to keep the tests more on topic, we could instead make the linker.txtar test set up a git repo with one commit, and do the similar [!short] env OUR_LINKER_CACHE=/some/empty/dir line to always patch and build a new linker with go test, but not go test -short. I think I prefer it that way, to keep all the linker stuff under one test.

@pagran
Copy link
Member Author

pagran commented Feb 8, 2023

Without --git-dir flag:

--- FAIL: TestScript (0.17s)
    --- FAIL: TestScript/linker (3.53s)
        testscript.go:429: WORK=/tmp/go-test-script1447969318/script-linker
            > [!short] [exec:git] exec git init -q
            > [!short] [exec:git] env GARBLE_CACHE_DIR=$WORK/linker-cache
            > garble build
            [stderr]
            # test/main
            cannot get modified linker: expected 2 applied patches, actually 0:

            Skipped patch 'cmd/link/internal/ld/pcln.go'.
            Skipped patch 'cmd/link/internal/ld/pcln.go'.

            exit status 2
            [exit status 1]
            FAIL: testdata/script/linker.txtar:4: unexpected command failure

FAIL
exit status 1
FAIL    mvdan.cc/garble 3.720s

@mvdan mvdan merged commit b0f8dfb into burrowers:master Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants