Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@bamarsha
Copy link
Contributor

@bamarsha bamarsha commented Jun 19, 2020

In microsoft/qsharp-compiler#468, I changed the compiler's representation of file IDs from AbsolutePath to LocalPath to fix a bug. This PR is needed to update a few places in IQ# that assumed that the file IDs were AbsolutePaths.

Although, actually, instead of using LocalPath explicitly (and needing to update it again if the file ID representation changes in the future), I would rather use CompilationUnitManager.TryGetFileId in all cases. The Try-ness of TryGetFileId makes it awkward to use in places where I believe it's guaranteed to succeed, though.

Since IQ# is accessing AbsolutePath without checks, I think that means the URIs in IQ# are guaranteed to be absolute. Are they also guaranteed to be file URIs? (IsAbsolute and IsFile being true are the only two things that TryGetFileId needs.)

@bettinaheim Would you be OK with a non-Try version of TryGetFileId being added to the compiler?

rmshaffer
rmshaffer previously approved these changes Jun 20, 2020
Copy link
Contributor

@rmshaffer rmshaffer left a comment

Choose a reason for hiding this comment

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

As far as I know, relying on TryGetFileId should be fine. It looks like these URIs are always absolute and are always files. (Well, the files don't necessarily exist, but at least they are paths to files. 😊) The tests should cover this code pretty thoroughly as well, such that it's probably safe to do this, though I'd love to get @cgranade's eyes on it as well.

@cgranade
Copy link
Contributor

It looks fine to me, but that's the part of the iqsharp codebase that @anpaz-msft would know better than I would. The only edge case I could imagine is if someone has a file named __snipets__.qs as part of a Q# that they then include from a package reference, such that library functions and operations may then have the same local path as the files used to compile snippets.

@bamarsha
Copy link
Contributor Author

The only edge case I could imagine is if someone has a file named __snipets__.qs as part of a Q# that they then include from a package reference, such that library functions and operations may then have the same local path as the files used to compile snippets.

I think that this edge case would exist independent of this PR - AbsolutePath vs LocalPath would be the difference between C:/foo%20bar/__snippets__.qs and C:\foo bar\__snippets__.qs, so they are both absolute local paths in that sense. The formatting is different but since all the URIs should be changed to LocalPath consistently, the potential for conflicts should be the same, I think.

@bamarsha bamarsha marked this pull request as ready for review June 22, 2020 21:45
@bamarsha bamarsha changed the title Use Uri.LocalPath instead of Uri.AbsolutePath for file IDs Use CompilationUnitManager.GetFileId instead of Uri.AbsolutePath for file IDs Jun 22, 2020
@rmshaffer
Copy link
Contributor

@SamarSha just FYI, the CI failure you're hitting is because the conda tests that I recently activated in the iqsharp CI aren't working correctly with -beta package versions. There is a workaround for this included in #177, which should be merged to master hopefully within a day. If you want to unblock your CI build here in the meantime, you could just apply this snippet directly to your branch:

# Add the prerelease NuGet feed if this isn't a release build.
if ("$Env:BUILD_RELEASETYPE" -ne "release") {
$NuGetDirectory = Resolve-Path ~
Write-Host "## Writing prerelease NuGet config to $NuGetDirectory ##"
"<?xml version=""1.0"" encoding=""utf-8""?>
<configuration>
<packageSources>
<add key=""qdk-alpha"" value=""https://pkgs.dev.azure.com/ms-quantum-public/Microsoft Quantum (public)/_packaging/alpha/nuget/v3/index.json"" protocolVersion=""3"" />
</packageSources>
</configuration>" | Out-File -FilePath $NuGetDirectory/NuGet.Config -Encoding utf8
}

rmshaffer
rmshaffer previously approved these changes Jun 25, 2020
@bamarsha bamarsha merged commit 6961049 into master Jun 25, 2020
@bamarsha bamarsha deleted the samarsha/spaces-in-path branch June 25, 2020 20:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants