USDLayerWriter fixes

Changes.md

@@ -1,7 +1,12 @@
 1.4.15.x (relative to 1.4.15.3)
 ========
 
+Fixes
+-----
 
+- USDLayerWriter :
+  - Fixed silent failures when unable to create the output file (#6197).
+  - Fixed leak of `usdLayerWriter:fileName` context variable.
 
 1.4.15.3 (relative to 1.4.15.2)
 ========

python/GafferUSDTest/USDLayerWriterTest.py

@@ -36,12 +36,15 @@
 
 import pathlib
 import unittest
+import os
+import subprocess
 
 import pxr.Usd
 
 import IECore
 import IECoreScene
 
+import Gaffer
 import GafferUSD
 import GafferScene
 import GafferSceneTest
@@ -231,5 +234,39 @@ def testChangePrimitiveType( self ) :
 		reader["fileName"].setValue( compositionFileName )
 		self.assertScenesEqual( reader["out"], spherePrimitive["out"], checks = self.allSceneChecks - { "sets" } )
 
+	def testNoContextLeaks( self ) :
+
+		sphere = GafferScene.Sphere()
+
+		layerWriter = GafferUSD.USDLayerWriter()
+		layerWriter["base"].setInput( sphere["out"] )
+		layerWriter["layer"].setInput( sphere["out"] )
+		layerWriter["fileName"].setValue( self.temporaryDirectory() / "layer.usda" )
+
+		with Gaffer.ContextMonitor( sphere ) as monitor :
+			layerWriter["task"].execute()
+
+		self.assertNotIn( "usdLayerWriter:fileName", monitor.combinedStatistics().variableNames() )
+
+	def testNoWritePermissions( self ) :
+
+		sphere = GafferScene.Sphere()
+
+		layerWriter = GafferUSD.USDLayerWriter()
+		layerWriter["base"].setInput( sphere["out"] )
+		layerWriter["layer"].setInput( sphere["out"] )
+		layerWriter["fileName"].setValue( self.temporaryDirectory() / "layer.usda" )
+
+		if os.name != "nt" :
+			self.temporaryDirectory().chmod( 444 )
+		else :
+			subprocess.check_call( [ "icacls", self.temporaryDirectory(), "/deny", "Users:(OI)(CI)(W)" ] )
+
+		with self.assertRaisesRegex( RuntimeError, 'Failed to export layer to "{}"'.format( layerWriter["fileName"].getValue() ) ) :
+			layerWriter["task"].execute()
+
+		if os.name == "nt" :
+			subprocess.check_call( [ "icacls", self.temporaryDirectory(), "/grant", "Users:(OI)(CI)(W)" ] )
+
 if __name__ == "__main__":
 	unittest.main()

src/GafferUSD/USDLayerWriter.cpp

@@ -241,6 +241,7 @@ USDLayerWriter::USDLayerWriter( const std::string &name )
 
 	NameSwitchPtr sceneSwitch = new NameSwitch( "__sceneSwitch" );
 	sceneSwitch->selectorPlug()->setValue( "${usdLayerWriter:fileName}" );
+	sceneSwitch->deleteContextVariablesPlug()->setValue( "usdLayerWriter:fileName" );
 	sceneSwitch->setup( basePlug() );
 	sceneSwitch->inPlugs()->getChild<NameValuePlug>( 0 )->valuePlug()->setInput( basePlug() );
 	sceneSwitch->inPlugs()->getChild<NameValuePlug>( 1 )->valuePlug()->setInput( layerPlug() );
@@ -363,9 +364,6 @@ void USDLayerWriter::executeSequence( const std::vector<float> &frames ) const
 	Context::EditableScope context( Context::current() );
 	for( const auto &fileName : { baseFileName, layerFileName } )
 	{
-		/// \todo Stop this context variable leaking out into the scene
-		/// evaluation. There is some talk of giving NameSwitch a feature to do
-		/// this for us.
 		context.set( "usdLayerWriter:fileName", &fileName );
 		sceneWriter()->taskPlug()->executeSequence( frames );
 	}
@@ -381,5 +379,8 @@ void USDLayerWriter::executeSequence( const std::vector<float> &frames ) const
 	createDiff( layer->GetPseudoRoot(), *layer, baseLayer->GetPseudoRoot(), *baseLayer );
 
 	createDirectories( outputFileName );
-	layer->Export( outputFileName );
+	if( !layer->Export( outputFileName ) )
+	{
+		throw IECore::Exception( fmt::format( "Failed to export layer to \"{}\"", outputFileName ) );
+	}
 }