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 Environment vars and empty daml.yamls in multi-ide #20306

Open
wants to merge 2 commits into
base: main-2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions sdk/compiler/daml-extension/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import * as fs from "fs";
import { ViewColumn, ExtensionContext } from "vscode";
import * as util from "util";
import fetch from "node-fetch";
import { DamlLanguageClient } from "./language_client";
import { DamlLanguageClient, EnvVars } from "./language_client";
import { resetTelemetryConsent, getTelemetryConsent } from "./telemetry";
import { WebviewFiles, getVRFilePath } from "./virtual_resource_manager";
import * as child_process from "child_process";
Expand Down Expand Up @@ -79,7 +79,7 @@ export async function activate(context: vscode.ExtensionContext) {
// Try to find a client for the virtual resource- if we can't, log to DevTools
let foundAClient = false;
for (let projectPath in damlLanguageClients) {
if (vrPath.startsWith(projectPath)) {
if (isPrefixOfVrPath(projectPath)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small fix missing from this pr

foundAClient = true;
damlLanguageClients[projectPath].virtualResourceManager.createOrShow(
title,
Expand Down Expand Up @@ -148,7 +148,7 @@ export async function activate(context: vscode.ExtensionContext) {
}

interface IdeManifest {
[multiPackagePath: string]: { [envVarName: string]: string };
[multiPackagePath: string]: EnvVars;
}

function parseIdeManifest(path: string): IdeManifest | null {
Expand Down Expand Up @@ -221,7 +221,7 @@ async function startLanguageServers(context: ExtensionContext) {
(await fileExists(rootPath + "/.envrc")) &&
vscode.extensions.getExtension("mkhl.direnv") == null;

if (envrcExistsWithoutExt && gradleSupport) {
if (envrcExistsWithoutExt) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

People use .envrc without gradle, this message should not be restricted to that

const warningMessage =
"Found an .envrc file but the recommended direnv VSCode extension was not installed. Daml IDE may fail to start due to missing environment." +
"\nWould you like to install this extension or attempt to continue without it?";
Expand Down Expand Up @@ -285,7 +285,7 @@ async function startLanguageServers(context: ExtensionContext) {
} else {
const languageClient = await DamlLanguageClient.build(
rootPath,
{},
process.env,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is why env vars broke

config,
consent,
context,
Expand Down
8 changes: 6 additions & 2 deletions sdk/compiler/daml-extension/src/language_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ namespace DamlKeepAliveRequest {

let damlRoot: string = path.join(os.homedir(), ".daml");

export interface EnvVars {
[envVarName: string]: string | undefined;
}

class UnsupportedFeature extends Error {
featureName: string;
sdkVersion: string;
Expand Down Expand Up @@ -72,7 +76,7 @@ export class DamlLanguageClient {

static async build(
rootPath: string,
envVars: { [envVarName: string]: string },
envVars: EnvVars,
config: vscode.WorkspaceConfiguration,
telemetryConsent: boolean | undefined,
_context: vscode.ExtensionContext,
Expand Down Expand Up @@ -301,7 +305,7 @@ export class DamlLanguageClient {
}
private static async createLanguageClient(
rootPath: string,
envVars: { [envVarName: string]: string },
envVars: EnvVars,
config: vscode.WorkspaceConfiguration,
telemetryConsent: boolean | undefined,
identifier: string | undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ module DA.Daml.Package.Config
, findMultiPackageConfig
, withMultiPackageConfig
, checkPkgConfig
, isDamlYamlContentForPackage
) where

import qualified DA.Daml.LF.Ast as LF
Expand All @@ -21,7 +22,7 @@ import DA.Daml.Project.Consts
import DA.Daml.Project.Types

import Control.Exception.Safe (throwIO, displayException)
import Control.Monad (when)
import Control.Monad (when, unless)
import Control.Monad.Extra (loopM)
import Control.Monad.Trans.Class (lift)
import Control.Monad.Trans.State.Lazy
Expand Down Expand Up @@ -168,19 +169,28 @@ fullDamlYamlFields = Set.fromList
, "typecheck-upgrades"
]

-- Determines if a daml.yaml is for defining a package (returns true) or simply for setting sdk-version (false)
isDamlYamlForPackage :: ProjectConfig -> Bool
isDamlYamlForPackage project =
case unwrapProjectConfig project of
A.Object obj -> any (`Set.member` fullDamlYamlFields) $ fmap A.toString $ A.keys obj
_ -> False

isDamlYamlContentForPackage :: T.Text -> Either ConfigError Bool
isDamlYamlContentForPackage projectContent =
isDamlYamlForPackage <$> readProjectConfigPure projectContent

withPackageConfig :: ProjectPath -> (PackageConfigFields -> IO a) -> IO a
withPackageConfig projectPath f = do
project <- readProjectConfig projectPath
-- If the config only has the sdk-version, it is "valid" but not usable for package config. It should be handled explicitly
case unwrapProjectConfig project of
A.Object (fmap A.toString . A.keys -> strKeys) | all (`Set.notMember` fullDamlYamlFields) strKeys ->
throwIO $ ConfigFileInvalid "project" $ Y.InvalidYaml $ Just $ Y.YamlException $
projectConfigName ++ " is a packageless daml.yaml, cannot be used for package config."
_ -> pure ()

pkgConfig <- either throwIO pure (parseProjectConfig project)
pkgConfig' <- overrideSdkVersion pkgConfig
f pkgConfig'
project <- readProjectConfig projectPath
-- If the config only has the sdk-version, it is "valid" but not usable for package config. It should be handled explicitly
unless (isDamlYamlForPackage project) $
throwIO $ ConfigFileInvalid "project" $ Y.InvalidYaml $ Just $ Y.YamlException $
projectConfigName ++ " is a packageless daml.yaml, cannot be used for package config."

pkgConfig <- either throwIO pure (parseProjectConfig project)
pkgConfig' <- overrideSdkVersion pkgConfig
f pkgConfig'

-- Traverses up the directory tree from current project path and returns the project path of the "nearest" project.yaml
-- Stops at root, but also won't pick any files it doesn't have permission to search
Expand Down
47 changes: 36 additions & 11 deletions sdk/compiler/damlc/lib/DA/Cli/Damlc/Command/MultiIde/Handlers.hs
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,13 @@ import Data.Foldable (traverse_)
import Data.List (find, isInfixOf, isSuffixOf)
import qualified Data.Map as Map
import Data.Maybe (fromMaybe, mapMaybe)
import qualified Data.Set as Set
import qualified Data.Text.IO as T
import qualified Language.LSP.Types as LSP
import qualified Language.LSP.Types.Lens as LSP
import System.Exit (exitSuccess)
import System.FilePath.Posix (takeDirectory, takeExtension, takeFileName)
import System.FilePath ((</>))

parseCustomResult :: Aeson.FromJSON a => String -> Either LSP.ResponseError Aeson.Value -> Either LSP.ResponseError a
parseCustomResult name =
Expand Down Expand Up @@ -146,26 +149,31 @@ subIdeMessageHandler miState unblock ide bs = do
logDebug miState $ "Backwarding response to " <> show method <> ":\n" <> show msg
sendClient miState msg

-- Returns whether the message should be forwarded to IDE (We do not want to forward open/close messages on a daml.yaml)
handleOpenFilesNotification
:: forall (m :: LSP.Method 'LSP.FromClient 'LSP.Notification)
. MultiIdeState
-> LSP.NotificationMessage m
-> FilePath
-> IO ()
-> IO Bool
handleOpenFilesNotification miState mess path = atomically $ case (mess ^. LSP.method, takeFileName path) of
(LSP.STextDocumentDidOpen, name) | ".daml" `isSuffixOf` name -> do
home <- getSourceFileHome miState path
addOpenFile miState home $ DamlFile path
pure True
(LSP.STextDocumentDidClose, name) | ".daml" `isSuffixOf` name -> do
home <- getSourceFileHome miState path
removeOpenFile miState home $ DamlFile path
-- Also remove from the source mapping, in case project structure changes while we're not tracking the file
sourceFileHomeHandleDamlFileDeleted miState path
(LSP.STextDocumentDidOpen, "daml.yaml") ->
setDamlYamlOpen miState (PackageHome $ takeDirectory path) True
(LSP.STextDocumentDidClose, "daml.yaml") ->
setDamlYamlOpen miState (PackageHome $ takeDirectory path) False
_ -> pure ()
pure True
(LSP.STextDocumentDidOpen, "daml.yaml") -> do
setDamlYamlOpen miState True $ PackageHome $ takeDirectory path
pure False
(LSP.STextDocumentDidClose, "daml.yaml") -> do
setDamlYamlOpen miState False $ PackageHome $ takeDirectory path
pure False
_ -> pure True

clientMessageHandler :: MultiIdeState -> IO () -> B.ByteString -> IO ()
clientMessageHandler miState unblock bs = do
Expand Down Expand Up @@ -254,10 +262,27 @@ clientMessageHandler miState unblock bs = do
LSP.FcDeleted -> do
shutdownIdeByHome miState home
handleRemovedPackageOpenFiles miState home
atomically $ onSubIde_ miState home $ \ideData -> ideData {ideDataIsFullDamlYaml = False}
LSP.FcCreated -> do
handleCreatedPackageOpenFiles miState home
rebootIdeByHome miState home
LSP.FcChanged -> rebootIdeByHome miState home
isFullDamlYaml <- shouldHandleDamlYamlChange <$> T.readFile (unPackageHome home </> "daml.yaml")
if isFullDamlYaml
then do
handleCreatedPackageOpenFiles miState home
rebootIdeByHome miState home
else shutdownIdeByHome miState home
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a daml.yaml containing only an sdk version is created, we shutdown its IDE and don't update any state

atomically $ onSubIde_ miState home $ \ideData -> ideData {ideDataIsFullDamlYaml = isFullDamlYaml}
LSP.FcChanged -> do
isFullDamlYaml <- shouldHandleDamlYamlChange <$> T.readFile (unPackageHome home </> "daml.yaml")
oldIdeData <- atomically $ onSubIde miState home $ \ideData -> (ideData {ideDataIsFullDamlYaml = isFullDamlYaml}, ideData)
if isFullDamlYaml
then do
when (not (ideDataIsFullDamlYaml oldIdeData) && Set.null (ideDataOpenFiles oldIdeData)) $
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to treat a daml.yaml file going from "only sdk-version" to a full package as though the daml.yaml was created.
We track the previous time the file changed in order to know if it has become valid in this run.

handleCreatedPackageOpenFiles miState home
rebootIdeByHome miState home
else do
when (not $ Set.null $ ideDataOpenFiles oldIdeData) $ handleRemovedPackageOpenFiles miState home
sendClient miState $ clearDiagnostics $ unPackageHome home </> "daml.yaml"
shutdownIdeByHome miState home
void $ updatePackageData miState

"multi-package.yaml" -> do
Expand Down Expand Up @@ -308,9 +333,9 @@ clientMessageHandler miState unblock bs = do

ForwardNotification mess (Single path) -> do
logDebug miState $ "single not on method " <> show meth <> " over path " <> path
handleOpenFilesNotification miState mess path
shouldForward <- handleOpenFilesNotification miState mess path
-- Notifications aren't stored, so failure to send can be ignored
sendSubIdeByPath miState path (castFromClientMessage msg)
when shouldForward $ sendSubIdeByPath miState path (castFromClientMessage msg)

ForwardNotification _ AllNotification -> do
logDebug miState $ "all not on method " <> show meth
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ sendPackageDiagnostic miState ideData = do
in traverse_ (sendClientSTM miState) $ makeMessage <$> files

onOpenFiles :: MultiIdeState -> PackageHome -> (Set.Set DamlFile -> Set.Set DamlFile) -> STM ()
onOpenFiles miState home f = modifyTMVarM (misSubIdesVar miState) $ \ides -> do
onOpenFiles miState home f = modifyTMVarM_ (misSubIdesVar miState) $ \ides -> do
let ideData = lookupSubIde home ides
ideData' = ideData {ideDataOpenFiles = f $ ideDataOpenFiles ideData}
sendPackageDiagnostic miState ideData'
Expand All @@ -52,9 +52,9 @@ removeOpenFile miState home file = do
unsafeIOToSTM $ logInfo miState $ "Removed open file " <> unDamlFile file <> " from " <> unPackageHome home
onOpenFiles miState home $ Set.delete file

setDamlYamlOpen :: MultiIdeState -> PackageHome -> Bool -> STM ()
setDamlYamlOpen miState home isOpen =
modifyTMVar (misSubIdesVar miState) $ Map.adjust (\ideData -> ideData {ideDataOpenDamlYaml = isOpen}) home
setDamlYamlOpen :: MultiIdeState -> Bool -> PackageHome -> STM ()
setDamlYamlOpen miState open home =
onSubIde_ miState home $ \ideData -> ideData {ideDataOpenDamlYaml = open}

isIdeDataOpen :: SubIdeData -> Bool
isIdeDataOpen SubIdeData {..} = not (Set.null ideDataOpenFiles) || ideDataOpenDamlYaml
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import Data.Foldable (traverse_)
import qualified Data.Map as Map
import Data.Maybe (catMaybes, isJust)
import qualified Data.Set as Set
import qualified Data.Text.Extended as T
import qualified Language.LSP.Types as LSP
import System.Directory (doesFileExist)
import System.FilePath.Posix ((</>))
Expand Down Expand Up @@ -56,9 +57,16 @@ updatePackageData miState = do
damlYamlExists <- doesFileExist $ ideRoot </> projectConfigName
if damlYamlExists
then do
logDebug miState "Found daml.yaml"
-- Treat a workspace with only daml.yaml as a multi-package project with only one package
deriveAndWriteMappings [PackageHome ideRoot] []
isFullDamlYaml <- shouldHandleDamlYamlChange <$> T.readFileUtf8 (ideRoot </> projectConfigName)
if isFullDamlYaml
then do
logDebug miState "Found daml.yaml"
-- Treat a workspace with only daml.yaml as a multi-package project with only one package
deriveAndWriteMappings [PackageHome ideRoot] []
else do
logDebug miState "Found daml.yaml, but not full package."
-- Treat as though no daml.yaml exists
deriveAndWriteMappings [] []
else do
logDebug miState "No daml.yaml found either"
-- Without a multi-package or daml.yaml, no mappings can be made. Passing empty lists here will give empty mappings
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,15 +249,15 @@ finishSdkInstallReporter :: MultiIdeState -> UnresolvedReleaseVersion -> IO ()
finishSdkInstallReporter miState ver = sendSdkInstallProgress miState ver InstallProgressEnd 100

untrackPackageSdkInstall :: MultiIdeState -> PackageHome -> IO ()
untrackPackageSdkInstall miState home = atomically $ modifyTMVar (misSdkInstallDatasVar miState) $
untrackPackageSdkInstall miState home = atomically $ modifyTMVar_ (misSdkInstallDatasVar miState) $
fmap $ \installData -> installData {sidPendingHomes = Set.delete home $ sidPendingHomes installData}

-- Unblock an ide's sdk from being installed if it was previously denied or failed.
allowIdeSdkInstall :: MultiIdeState -> PackageHome -> IO ()
allowIdeSdkInstall miState home = do
ePackageSummary <- packageSummaryFromDamlYaml home
forM_ ePackageSummary $ \ps ->
atomically $ modifyTMVar (misSdkInstallDatasVar miState) $ Map.adjust (\installData ->
atomically $ modifyTMVar_ (misSdkInstallDatasVar miState) $ Map.adjust (\installData ->
installData
{ sidStatus = case sidStatus installData of
SISDenied -> SISCanAsk
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ getSourceFileHome miState path = do
sourceFileHomeHandleDamlFileDeleted :: MultiIdeState -> FilePath -> STM ()
sourceFileHomeHandleDamlFileDeleted miState path = do
dirPath <- unsafeIOToSTM $ getDirectoryIfFile path
modifyTMVar (misSourceFileHomesVar miState) $ Map.delete dirPath
modifyTMVar_ (misSourceFileHomesVar miState) $ Map.delete dirPath

-- When a daml.yaml changes, all files pointing to it are invalidated in the cache
sourceFileHomeHandleDamlYamlChanged :: MultiIdeState -> PackageHome -> STM ()
sourceFileHomeHandleDamlYamlChanged miState home = modifyTMVar (misSourceFileHomesVar miState) $ Map.filter (/=home)
sourceFileHomeHandleDamlYamlChanged miState home = modifyTMVar_ (misSourceFileHomesVar miState) $ Map.filter (/=home)
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,16 @@ data SubIdeData = SubIdeData
, ideDataClosing :: Set.Set SubIdeInstance
, ideDataOpenFiles :: Set.Set DamlFile
, ideDataOpenDamlYaml :: Bool
, ideDataIsFullDamlYaml :: Bool
-- ^ We support daml.yaml files with only an sdk version. These should not be considered as packages
-- however, we still want to track data about these, as file changes can promote/demote them to/from packages
-- Thus we store this flag, rather than omitting from subIdes
, ideDataFailures :: [(UTCTime, T.Text)]
, ideDataDisabled :: IdeDataDisabled
}

defaultSubIdeData :: PackageHome -> SubIdeData
defaultSubIdeData home = SubIdeData home Nothing Set.empty Set.empty False [] IdeDataNotDisabled
defaultSubIdeData home = SubIdeData home Nothing Set.empty Set.empty False False [] IdeDataNotDisabled

lookupSubIde :: PackageHome -> SubIdes -> SubIdeData
lookupSubIde home ides = fromMaybe (defaultSubIdeData home) $ Map.lookup home ides
Expand Down
Loading