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

Add #getTemporaryDirectory to UnixPlatform and WinPlatform #13777

Merged
merged 10 commits into from
May 26, 2023

Conversation

gcorriga
Copy link
Contributor

Add #getTemporaryDirectory to UnixPlatform and WinPlatform, with related tests.

Do not add yet an abstract #getTemporaryDirectory to OSPlatform as macOS might require additional work to support NSTemporaryDirectory.

gcorriga added 2 commits May 21, 2023 11:09
…ted tests.

Do not add yet an abstract #getTemporaryDirectory to OSPlatform as macOS might require additional work to support NSTemporaryDirectory.
@MarcusDenker MarcusDenker added the Status: Need more work The issue is nearly ready. Waiting some last bits. label May 22, 2023
@jecisc
Copy link
Member

jecisc commented May 22, 2023

I am wondering what is the use of those new methods? We already have FileLocator temp, what is the difference?

@gcorriga
Copy link
Contributor Author

gcorriga commented May 22, 2023

@jecisc I was not aware of

FileLocator temp

Looking at the underlying behaviour it doesn’t appear to be correct - e.g. for Windows the TMP env variable should have precedence over TEMP.
I will rework my PR to move the correct behaviour in the right place.

@jecisc
Copy link
Member

jecisc commented May 22, 2023

Thank you! :)

@gcorriga gcorriga requested a review from tesonep May 22, 2023 19:20
@jecisc jecisc added Status: Tests passed please review! and removed Status: Need more work The issue is nearly ready. Waiting some last bits. labels May 23, 2023
@jecisc
Copy link
Member

jecisc commented May 23, 2023

The tests passed and now it improves the current implementation so it's way better!

The code seems good but I'm not an expert on this part of the system so another review might be nice

@Ducasse Ducasse merged commit fc8a6ff into pharo-project:Pharo12 May 26, 2023
@Rinzwind
Copy link
Contributor

The output of the Windows test running step in build 3 of pull request #16337 shows the implementation of #temp on WindowsResolver introduced in this pull request in commit 78e0652 is causing an error:

<testsuite name="Epicea-Tests" tests="55" timestamp="12:39:40.92 pm" seed="1053542408"  failures="0" errors="110" time="24.537">                                                            
	<testcase classname="Windows64.Epicea.Tests.EpAnnouncementsTest" name="testMonitorAnnouncesUpdateWhenDisabled" time="0.631">
		<error type="SymbolNotFoundError" message="Could not find symbol named: #GetTempPath2W searching in module: 'Kernel32'">
SymbolNotFoundError
Could not find symbol named: #GetTempPath2W searching in module: 'Kernel32'
TFFIBackend>>primLoadSymbol:module:
[…]
Win64Platform(WinPlatform)>>getTempPath:buffer:
Win64Platform(WinPlatform)>>getTempPath
WindowsResolver>>temp
[…]
EpTestLogBuilder>>newDirectory
[…]
EpAnnouncementsTest(EpMonitorIntegrationTest)>>setUp
		</error>

This might be due to the Jenkins agent not meeting the requirements for ‘GetTempPath2W’ (‘Windows 10 Build 20348’ / ‘Windows Server Build 20348’).

@gcorriga: Could ‘GetTempPathW’ be used instead?

@gcorriga
Copy link
Contributor Author

@Rinzwind I believe it should be possible to use GetTempPathW. No issues from me if you want to go ahead with this change.

Please note, I am on paternity break until April 2nd so I won't be able to help much.

@Rinzwind
Copy link
Contributor

OK, I have opened a pull request to change it: pull request #16342.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants