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: ci compile errors #20

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

fix: ci compile errors #20

wants to merge 9 commits into from

Conversation

polymo1
Copy link
Member

@polymo1 polymo1 commented Apr 28, 2024

Closes: #19

I've fixed the build errors, but the test errors aren't passing. Both fixes are provided by VS Code + rust-analyzer.

I also added workflow_dispatch: to the workflow to allow manual runs, which I found to be needed in the scenario of committing to a fork before enabling workflows.

@polymo1 polymo1 requested a review from JoeMatt April 28, 2024 00:04
@polymo1 polymo1 self-assigned this Apr 28, 2024
@polymo1 polymo1 added bug Something isn't working devops CI/CD labels Apr 28, 2024
Copy link
Member

@junepark678 junepark678 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving because build error stops it from actually building instead of tests... which seems to be broken

@polymo1 polymo1 marked this pull request as draft April 28, 2024 01:46
@polymo1
Copy link
Member Author

polymo1 commented Apr 28, 2024

Noticing an issue in regards to the second test in lib.rs

It panics with:

assertion `left == right` failed
  left: OtherAlg(OtherAlgorithmIdentifier { algorithm_type: ObjectIdentifier { components: [2, 16, 840, 1, 101, 3, 4, 2, 1] }, params: Some([5, 0]) })
 right: Sha1

@polymo1
Copy link
Member Author

polymo1 commented Apr 28, 2024

It might have been right in front of me, maybe I need to try a Sha1 cert...

Using a Sha1 cert did NOT work.

@polymo1 polymo1 removed their assignment Apr 28, 2024
@polymo1
Copy link
Member Author

polymo1 commented Apr 28, 2024

I'm lost at this point, it looks like it's prompting for a Sha1 cert, but when using a Sha1 cert the exact same error occurs. I fixed the build errors, it can't get past the test though.

@polymo1
Copy link
Member Author

polymo1 commented Apr 28, 2024

@JoeMatt thoughts?

@JoeMatt
Copy link
Member

JoeMatt commented Apr 28, 2024

Looks like the issue is that the test code is incomplete.

I would suggest removing it or fixing it.

Since I'm new to Rust, I asked an LLM to clean up the warnings VSCode gave me about using unwrap and the parameters, and it gave me this.

    #[test]
    fn sign_app() {
        crate::tests::logger();

        let p12_content = std::fs::read("TODO.p12").expect("Failed to read TODO.p12 file");

        super::sign_app(
            "src/test.app",
            "com.wesbryie.test",
            &p12_content, // Pass the content as a slice reference instead of an owned vector
            "",
        )
        .expect("Signing app failed.");
    }

I replaced the .app and BUNDLE_ID parameters with the values you used in the other test.

I assume this needs the P12?

@polymo1
Copy link
Member Author

polymo1 commented Apr 28, 2024

In apple-codesign-wrapper/lib.rs I just commented out the test because its giving some oddball error which cannot be solved by me even with the suggestion above. (cc: @JoeMatt )

The check is failing because xcsession.rs expects user input, but I feel that a test should not have user input. Is this safe to comment out?

@JJTech0130 For fetch_anisette_ssc test, I feel it should be commented out because omnisette is obsolete when compared to anisette-server-v3. Do you agree with this? It fails with a directory not found error regarding the anisette_test directory.

@polymo1 polymo1 self-assigned this Apr 28, 2024
@JJTech0130
Copy link
Member

JJTech0130 commented Apr 28, 2024

Actually, I think that omnisette[storeservicescore] is ideal for use cases where it's just a single instance on-device, and should be kept working if possible-- the only real downside was memory leaks, right?

As for the directory not found error: pretty sure that's the output directory for the anisette.pb file, right? There's some kind of temporary test or build directory created by Rust automatically, right? Maybe you could put it in there?

Another thing, is there anywhere we can move test.app in the repo other than in that main src/ folder? Some kind of test_assets or something folder? Just so that it's clear that it is only an arbitrary test asset.

@JoeMatt
Copy link
Member

JoeMatt commented Apr 28, 2024

Actually, I think that omnisette[storeservicescore] is ideal for use cases where it's just a single instance on-device, and should be kept working if possible-- the only real downside was memory leaks, right?

As for the directory not found error: pretty sure that's the output directory for the anisette.pb file, right? There's some kind of temporary test or build directory created by Rust automatically, right? Maybe you could put it in there?

Another thing, is there anywhere we can move test.app in the repo other than in that main src/ folder? Some kind of test_assets or something folder? Just so that it's clear that it is only an arbitrary test asset.

Agreed.

I'm just learning Rust, but it looks like they follow a similar pattern to SPM, where tests go into a /tests folder, etc.

The way this test function is defined, I think, makes it required to be in the same file.

The LLM also suggested refactoring that method into a separate method and file.

Maybe ask a GPT 🙂

I've been using LM Studio with models downloaded from HuggingFace for free local LLM that you can then connect to other IDEs.

@TaeHagen
Copy link
Contributor

I'm not sure if it's merged fully yet, but I implemented Anisette v3 into omnisette, so it's not completely obsolete. The anisette_test directory is used by my v3 impl to store the returned device data/provisioning keys. I don't know what it was for in the older anisette impls, or what omnisette was even supposed to do before v3 lol. That is where I store the anisette.pb file.

Tests can go in a test module, either in a different file or in the same one. Just note them with #cfg[test] and functions with #[test]

@JoeMatt
Copy link
Member

JoeMatt commented May 2, 2024

How about we shelve that test to get this PR in and make a new ticket to add more test coverage?

@polymo1 polymo1 removed their assignment Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working devops CI/CD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix CI
5 participants