[post build lint] Check for absolute paths #172

autoantwort · 2021-08-25T16:14:02Z

Prevents microsoft/vcpkg#19720

src/vcpkg/postbuildlint.cpp

Neumann-A · 2021-08-25T18:12:53Z

vcpkg_fixup_pkgconfig.cmake sets the cmake variable VCPKG_FIXUP_PKGCONFIG_CALLED so this could be moved into ports.cmake instead. Or make vcpkg read that additional variable.

autoantwort · 2021-08-25T18:20:24Z

vcpkg_fixup_pkgconfig.cmake sets the cmake variable VCPKG_FIXUP_PKGCONFIG_CALLED so this could be moved into ports.cmake instead. Or make vcpkg read that additional variable.

Yeah possible, but theoretically one could pass the params RELEASE_FILES and DEBUG_FILES to vcpkg_fixup_pkgconfig so that it misses some files that are then not fixed.

strega-nil-ms · 2021-09-08T00:02:56Z

I think I agree with @Neumann-A here; or at least, I think that'd be a better check. For here, I want to check for absolute paths generally, since someone might not set prefix in their pkgconfig file but it's still incorrect and needs to be fixup'd.

autoantwort · 2021-09-24T09:20:27Z

For here, I want to check for absolute paths generally, since someone might not set prefix in their pkgconfig file but it's still incorrect and needs to be fixup'd.

That was now also proposed in microsoft/vcpkg#20321

autoantwort · 2021-09-24T15:56:52Z

Same PR but now checks for absolute paths

src/vcpkg/postbuildlint.cpp

strega-nil-ms · 2021-10-06T18:41:34Z

@autoantwort would you mind not force pushing to one commit? It makes it really difficult to review, and we're going to squash and merge at the end anyways.

strega-nil-ms

Some more things to make sure this works well.

src/vcpkg/postbuildlint.cpp

autoantwort · 2021-10-07T22:20:52Z

@autoantwort would you mind not force pushing to one commit? It makes it really difficult to review, and we're going to squash and merge at the end anyways.

Yeah sure. I normally use gitlab where you have compare to previous version, but github does not support that.

src/vcpkg/postbuildlint.cpp

Co-authored-by: nicole mazzuca <[email protected]>

include/vcpkg/base/util.h

src/vcpkg/postbuildlint.cpp

# Conflicts: # locales/messages.en.json

BillyONeal

I agree with @vicroms that this needs e2e tests.

I prepared a commit with fixes for most of what I noticed I would like to see taken but it's enough change that I don't want to just push it on you:

https://github.com/BillyONeal/vcpkg-tool/tree/check-pkg-config-files /

BillyONeal@4eef012

src/vcpkg/postbuildlint.cpp

BillyONeal · 2022-11-17T04:45:20Z

include/vcpkg/base/strings.h

@@ -218,6 +220,8 @@ namespace vcpkg::Strings

    Optional<StringView> find_at_most_one_enclosed(StringView input, StringView left_tag, StringView right_tag);

+    bool contains_any_ignoring_c_comments(const std::string& source, View<StringView> to_find);


source should be a StringView

src/vcpkg/postbuildlint.cpp

BillyONeal · 2022-11-17T05:54:59Z

src/vcpkg/postbuildlint.cpp

+                    for (size_t offset = 0;;)
+                    {
+                        const auto index = path_and_contents.second.find(path, offset);
+                        if (index == std::string::npos) return false;
+                        { // .py, .sh, .cmake or .pc file
+                            const auto before = path_and_contents.second.find_last_of("\n#", index);
+                            if (before == std::string::npos) return true;
+                            if (path_and_contents.second[before] == '\n') return true; // not a comment
+                        }
+                        offset = index + path.size();
+                    }


This part looks extractable like contains_any_ignoring_c_comments. And it should handle #s not at the start of a line.

It does that

src/vcpkg/base/strings.cpp

BillyONeal · 2022-11-17T08:08:17Z

include/vcpkg/base/messages.h

@@ -1088,6 +1090,11 @@ namespace vcpkg
                    "One or more {vendor} credential providers failed to authenticate. See '{url}' for more details "
                    "on how to provide credentials.");
    DECLARE_MESSAGE(FeedbackAppreciated, (), "", "Thank you for your feedback!");
+    DECLARE_MESSAGE(FilesContainAbsolutePath,


This needs to tell the user about the policy now.

I thought the policy should only be used internally to that merging this PR does the break the world. Imho we should not tell the users how to ignore warnings that warns before broken packages.

autoantwort · 2022-11-17T09:59:44Z

I have tested your patch. It gives a lot of false positives (probably a bug somewhere) and it is 4 times slower then the current implementation (500 ms vs 2.2 seconds for qtbase)

…Billy

BillyONeal · 2022-11-17T18:53:13Z

I have tested your patch. It gives a lot of false positives (probably a bug somewhere)

For example? Note that the raw string literal bug effectively causes everything after the raw string literal to be considered part of it most of the time. I also didn't write tests around the python one so there are likely issues there.

it is 4 times slower then the current implementation (500 ms vs 2.2 seconds for qtbase)

Yeah I did no perf testing yet. What I did gives up on the potential simd that find_first_of does. I just spent over 4 hours trying to prove correctness of this PR yesterday and that's as far as I got. As written the cyclomatic complexity was ... high :)

autoantwort · 2022-11-18T19:19:00Z

For example?

The problem was

            if (!file_contains_absolute_paths(fs, file, stringview_paths))
            {
                failing_files.push_back(file); // mark as failing file if it does not contains an absolute paths
            }

I just spent over 4 hours trying to prove correctness of this PR yesterday and that's as far as I got. As written the cyclomatic complexity was ... high :)

Thank you for the effort :) ❤️

BillyONeal · 2022-11-18T19:55:40Z

OMG I flipped the answer? 🤣😂🤣

Code from BillyONeal@4eef012

BillyONeal · 2022-12-07T21:08:15Z

@vicroms outstanding comment is that he wants an e2e test for the failing cases.

BillyONeal

@vicroms Can you OK this?

BillyONeal · 2023-01-20T20:58:54Z

Thank you for your efforts in making a better package catalog, and a better vcpkg <3

Neumann-A reviewed Aug 25, 2021

View reviewed changes

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

autoantwort force-pushed the check-pkg-config-files branch from 6caebcb to a21a9c5 Compare August 25, 2021 18:13

autoantwort force-pushed the check-pkg-config-files branch from a21a9c5 to 2d4aab1 Compare September 24, 2021 15:56

autoantwort changed the title ~~[post build lint] Check if vcpkg_fixup_pkgconfig was called~~ [post build lint] Check for absolute paths Sep 24, 2021

autoantwort mentioned this pull request Sep 25, 2021

build_info.cmake: Add VCPKG_POLICY_SKIP_PKGCONFIG_LOCATION_CHECK microsoft/vcpkg#20344

Closed

strega-nil-ms reviewed Sep 27, 2021

View reviewed changes

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

autoantwort mentioned this pull request Oct 1, 2021

Remove absolute paths from installed files microsoft/vcpkg#20469

Closed

38 tasks

autoantwort force-pushed the check-pkg-config-files branch from 2d4aab1 to 67534e1 Compare October 6, 2021 08:08

[post build lint] Check for absolute paths

863231d

autoantwort force-pushed the check-pkg-config-files branch from 67534e1 to 863231d Compare October 6, 2021 10:37

strega-nil-ms suggested changes Oct 6, 2021

View reviewed changes

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

autoantwort commented Oct 7, 2021

View reviewed changes

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

autoantwort and others added 3 commits October 8, 2021 00:25

Apply suggestions from code review

15778b6

Co-authored-by: nicole mazzuca <[email protected]>

Fix build and logic

9212d1b

Merge branch 'main' into check-pkg-config-files

326672e

autoantwort force-pushed the check-pkg-config-files branch from 0689e6b to 326672e Compare October 12, 2021 19:26

strega-nil-ms approved these changes Oct 18, 2021

View reviewed changes

include/vcpkg/base/util.h Outdated Show resolved Hide resolved

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved

strega-nil-ms and others added 2 commits October 18, 2021 12:39

nicole's CRs

cef8cb0

fix build

761dc17

autoantwort mentioned this pull request Oct 23, 2021

[many ports] call vcpkg_fixup_pkgconfig() microsoft/vcpkg#20953

Merged

autoantwort added 3 commits October 29, 2021 20:29

Merge branch 'main' into check-pkg-config-files

7fd595a

Ignore dot in extension

7e9fdd0

Merge branch 'main' into check-pkg-config-files

f58d9ae

autoantwort and others added 2 commits November 3, 2022 19:57

Merge branch 'main' into check-pkg-config-files

0a5c013

# Conflicts: # locales/messages.en.json

Merge remote-tracking branch 'origin/main' into check-pkg-config-files

028dd92

BillyONeal requested changes Nov 17, 2022

View reviewed changes

autoantwort added 4 commits November 17, 2022 11:25

Fix detection of the end of raw strings literals and copy tests from …

a2a42d5

…Billy

No optimization

4092884

Fix paths on windows

22c3d0e

No need for .native()

c1188c6

autoantwort added 6 commits November 19, 2022 01:25

More tests and less memory usage

f8b77a2

Code from BillyONeal@4eef012

Don't search for downloads folder

0937195

Ignore hash comments in .conf files

bc23f1f

Fix formatting

298a2f4

Fix buffer overflow

4fabb06

Merge branch 'main' into check-pkg-config-files

ed7e9c8

BillyONeal approved these changes Dec 7, 2022

View reviewed changes

Add e2e tests

cc3f741

BillyONeal approved these changes Dec 8, 2022

View reviewed changes

vicroms approved these changes Dec 15, 2022

View reviewed changes

autoantwort mentioned this pull request Jan 8, 2023

[DO NOT MERGE] test absolute paths microsoft/vcpkg#28802

Closed

autoantwort added 2 commits January 18, 2023 13:41

Merge branch 'main' into check-pkg-config-files

ccf6b23

Adapt ee7bb98

5222349

autoantwort force-pushed the check-pkg-config-files branch from 7798f61 to 5222349 Compare January 18, 2023 12:50

BillyONeal merged commit fcfbfa4 into microsoft:main Jan 20, 2023

autoantwort deleted the check-pkg-config-files branch January 20, 2023 21:03

BillyONeal mentioned this pull request Feb 27, 2023

Consider adding a post-install check for any [VCPKG_ROOT]/packages paths in non-binary files installed microsoft/vcpkg#20321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[post build lint] Check for absolute paths #172

[post build lint] Check for absolute paths #172

autoantwort commented Aug 25, 2021 •

edited

Loading

Neumann-A commented Aug 25, 2021

autoantwort commented Aug 25, 2021

strega-nil-ms commented Sep 8, 2021

autoantwort commented Sep 24, 2021

autoantwort commented Sep 24, 2021

strega-nil-ms commented Oct 6, 2021

strega-nil-ms left a comment

autoantwort commented Oct 7, 2021

BillyONeal left a comment

BillyONeal Nov 17, 2022

BillyONeal Nov 17, 2022

autoantwort Nov 18, 2022

BillyONeal Nov 17, 2022

autoantwort Nov 18, 2022

autoantwort commented Nov 17, 2022

BillyONeal commented Nov 17, 2022

autoantwort commented Nov 18, 2022

BillyONeal commented Nov 18, 2022

BillyONeal commented Dec 7, 2022

BillyONeal left a comment

BillyONeal commented Jan 20, 2023

		@@ -218,6 +220,8 @@ namespace vcpkg::Strings

		Optional<StringView> find_at_most_one_enclosed(StringView input, StringView left_tag, StringView right_tag);

		bool contains_any_ignoring_c_comments(const std::string& source, View<StringView> to_find);

[post build lint] Check for absolute paths #172

[post build lint] Check for absolute paths #172

Conversation

autoantwort commented Aug 25, 2021 • edited Loading

Neumann-A commented Aug 25, 2021

autoantwort commented Aug 25, 2021

strega-nil-ms commented Sep 8, 2021

autoantwort commented Sep 24, 2021

autoantwort commented Sep 24, 2021

strega-nil-ms commented Oct 6, 2021

strega-nil-ms left a comment

Choose a reason for hiding this comment

autoantwort commented Oct 7, 2021

BillyONeal left a comment

Choose a reason for hiding this comment

BillyONeal Nov 17, 2022

Choose a reason for hiding this comment

BillyONeal Nov 17, 2022

Choose a reason for hiding this comment

autoantwort Nov 18, 2022

Choose a reason for hiding this comment

BillyONeal Nov 17, 2022

Choose a reason for hiding this comment

autoantwort Nov 18, 2022

Choose a reason for hiding this comment

autoantwort commented Nov 17, 2022

BillyONeal commented Nov 17, 2022

autoantwort commented Nov 18, 2022

BillyONeal commented Nov 18, 2022

BillyONeal commented Dec 7, 2022

BillyONeal left a comment

Choose a reason for hiding this comment

BillyONeal commented Jan 20, 2023

autoantwort commented Aug 25, 2021 •

edited

Loading