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

LS: Look for unmanaged core in Scarb #6248

Merged
merged 1 commit into from
Aug 27, 2024
Merged

LS: Look for unmanaged core in Scarb #6248

merged 1 commit into from
Aug 27, 2024

Conversation

mkaput
Copy link
Member

@mkaput mkaput commented Aug 20, 2024

fixes #6235


This change is Reviewable

@mkaput mkaput linked an issue Aug 20, 2024 that may be closed by this pull request
@mkaput mkaput force-pushed the spr/main/baf78a5f branch 2 times, most recently from f469286 to eef5147 Compare August 21, 2024 07:33
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae, @Draggu, and @piotmag769)

Copy link
Collaborator

@Arcticae Arcticae left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Draggu and @piotmag769)

@mkaput mkaput changed the base branch from spr/main/c5cd2e07 to main August 22, 2024 10:46
@mkaput mkaput force-pushed the spr/main/baf78a5f branch from eef5147 to 583c5e9 Compare August 22, 2024 10:46
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Draggu and @piotmag769)

Copy link
Collaborator

@Draggu Draggu left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 5 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @piotmag769)

Copy link
Collaborator

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

:lgtm: feel free to resolve everything and answer later

Reviewed 2 of 5 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mkaput)


crates/cairo-lang-language-server/src/project/unmanaged_core_crate.rs line 37 at r3 (raw file):

        .or_else(|| find_scarb_managed_core(scarb))
        .or_else(|| {
            if cfg!(feature = "testing") {

Is this feature set by compiler team?


crates/cairo-lang-language-server/src/project/unmanaged_core_crate.rs line 133 at r3 (raw file):

    };

    static CACHE: OnceLock<Option<PathBuf>> = OnceLock::new();

Smart! 💡


crates/cairo-lang-language-server/src/toolchain/scarb.rs line 56 at r3 (raw file):

                if path.is_none() {
                    if self.is_silent {
                        // If we are in silent mode, then missing Scarb is probably dealt with

Why?


crates/cairo-lang-language-server/src/toolchain/scarb.rs line 83 at r3 (raw file):

                // initialized yet.
                //
                // This maintains a good UX for the following scenario (timeline):

Could you elaborate on that? I don't think I quite get it

@mkaput mkaput added this pull request to the merge queue Aug 26, 2024
Copy link
Member Author

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @piotmag769)


crates/cairo-lang-language-server/src/project/unmanaged_core_crate.rs line 37 at r3 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Is this feature set by compiler team?

This is only set in our tests, here:

cairo-lang-language-server = { path = ".", features = ["testing"] }


crates/cairo-lang-language-server/src/toolchain/scarb.rs line 56 at r3 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Why?

The whole idea of silent mode is that we're using Scarb but in a not obligatory way, i.e. nothing's wrong if Scarb is missing etc. This naturally means that we cannot shout at the user like we'd do in normal mode because this might end up being misleading.

A prime example are the E2E tests, where we don't have Scarb, so we expect the scarb-managed-core branch to fail, but right after we're using detect_corelib. We do not want to produce the missing-scarb notification in such scenario.


crates/cairo-lang-language-server/src/toolchain/scarb.rs line 83 at r3 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Could you elaborate on that? I don't think I quite get it

A silently failing scarb-managed-core flow that would happen before scarb metadata call, could block the missing-scarb notificaiton from popping up when that scarb metadata is run

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 26, 2024
Copy link
Collaborator

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mkaput)

@mkaput mkaput added this pull request to the merge queue Aug 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Aug 26, 2024
@mkaput mkaput added this pull request to the merge queue Aug 27, 2024
Merged via the queue into main with commit 26ce43c Aug 27, 2024
88 checks passed
@mkaput mkaput deleted the spr/main/baf78a5f branch August 27, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Look for unmanaged core in Scarb
5 participants