fix Score examples and Platform.defaultTempDir on OSX #4221

geoffroymontel · 2018-12-21T17:01:30Z

Purpose and Motivation

Fixes #1271

Types of changes

documentation of Score
changed Platform.defaultTempDir because /tmp is not writeable on OSX and is used in Score rendering

Checklist

All previous tests are passing
Tests were updated or created to address changes in this PR, and tests are passing
Updated documentation, if necessary
This PR is ready for review

Remaining Work

patrickdupuis · 2019-01-03T23:29:38Z

SCClassLibrary/Platform/Platform.sc

+		var dir;
+		dir = Platform.userAppSupportDir ++ "/tmp/";
+		dir.mkdir;
+		^dir;


This change causes the defaultTempDir to no longer be /tmp on Linux. Maybe this is a good thing? It's worth discussing.

I'm guessing that Windows won't be too happy about ++ "/tmp/". You should make use of +/+. Also, we should keep using File.exists so that we don't call .mkdir every time.

Ignore my comment about Windows. I didn't really understand how Platform works on Windows.

gusano

@geoffroymontel It seems that /tmp not being writeable is a macOS-only issue (and from a basic search it seems to only happen on El Capitan although I'm not 100% sure about that).
I'd strongly suggest to not change default /tmp folder on linux but rather use that workaround for macOS only.

…tmp for Linux

geoffroymontel · 2019-01-04T11:08:24Z

@gusano @patrickdupuis thanks for your reviews ! I reverted Platform.defaultTempDir to /tmp on Linux and changed it in OSX only.

gusano · 2019-01-04T20:44:42Z

SCClassLibrary/Platform/osx/OSXPlatform.sc

+
+	defaultTempDir {
+		var dir;
+		dir = Platform.userAppSupportDir +/+ "tmp";


@geoffroymontel Now I believe that the trailing / is missing here

@gusano I've tested the code and it seems to work without the trailing slash. Is it ok for you ?

why would the trailing slash be necessary? @gusano

@brianlheim it seems that Platform.defaultTempDir() already returns a trailing slash by default so it might be better to keep it that way to not break existing code?

@geoffroymontel I'm surprised that it works, I cannot test it now but my guess is that the file might end being
"/path-to-app-support-dir/tmpasScore-Help.aif"

Sorry @gusano you're right, I had not fully tested !
I added a trailing slash like on Linux

ok, fair, did not know that. everyone should be using +/+ to concatenate paths though precisely to not get burned by this kind of detail. i believe other platform directory methods return paths without the trailing slash.

gusano

@geoffroymontel thanks

mossheim · 2019-01-16T03:18:07Z

SCClassLibrary/Platform/osx/OSXPlatform.sc

+		var dir;
+		// ensure trailing slash
+		dir = Platform.userAppSupportDir +/+ "tmp/";
+		if (File.exists(dir).not, { dir.mkdir; });


please fix formatting - if(File.exists(dir).not) { dir.mkdir };

Wow, I did not know this syntax was allowed !

if (bool) { "true".postln } { "false".postln }

I always thought parenthesis around functions were needed, because if was a function that would take a boolean and two functions.

if (bool, { "true".postln }, { "false".postln });

This syntax is not mentionned in http://doc.sccode.org/Reference/Control-Structures.html

Should I fill a separate PR for that ?

also, why no space between if and the condition ?
is there a guide somewhere for the codebase formatting ?

There's this WIP guide. I seems to contradict what is being suggested here: https://github.com/supercollider/supercollider/wiki/Code-style-guidelines#whitespace-1

Thanks Patrick. I think I will rewrite it as :

if (File.exists(dir).not) { dir.mkdir };

then

After the discussion on Slack, the style guide mentions a space after if for the C++ codebase, but no space for the SCLang codebase.

Then I will use

if(File.exists(dir).not) { dir.mkdir };

changes done

mossheim · 2019-01-16T03:18:29Z

HelpSource/Classes/Score.schelp

 // render the pattern to aiff (4 beats)
-
-p.render("asScore-Help.aif", 4.0);
+p.render(Platform.defaultTempDir ++ "asScore-Help.aif", 4.0);


use +/+ here

Definitely, I will do that

mossheim · 2019-01-16T03:18:48Z

HelpSource/Classes/Score.schelp

 f.write(g.asCompileString);
 f.close;
 )

-z = Score.newFromFile("score-test");
+z = Score.newFromFile(Platform.defaultTempDir ++ "score-test");


use +/+ here and in the other spots above (i count 7)

Thanks, and sorry this PR needs so much review for so little code

alln4tural · 2019-01-16T12:59:15Z

if (File.exists(dir).not, { dir.mkdir; }); conforms to the main syntax given in the docs: if (expr, trueFunc, falseFunc); Or am I misunderstanding your point? Cheers, eddi

…

-- https://soundcloud.com/all-n4tural https://alln4tural.bandcamp.com

geoffroymontel · 2019-01-16T13:21:27Z

@alln4tural my point was that I wrote initially

if (File.exists(dir).not, { dir.mkdir; });

and Brian suggested I wrote

if(File.exists(dir).not) { dir.mkdir };

instead, which is a style more in line with other programming languages, but I did not know SuperCollider allowed that.

alln4tural · 2019-01-16T17:00:00Z

Ah, sorry! That wasn't apparent from the email I got.

…

-- https://soundcloud.com/all-n4tural https://alln4tural.bandcamp.com

mossheim · 2019-01-17T03:25:48Z

SCClassLibrary/Platform/osx/OSXPlatform.sc

+
+	defaultTempDir {
+		var dir;
+		// ensure trailing slash


one last thing i didn't notice before -- this comment explains 'what', but not 'why' - can you please add a note that this is due to backwards compatibility, referencing this PR (#4221), so that future generations are not confused? sorry i didn't catch this before, after this it is definitely ready to merge. :)

Done !
I think it was the worst number of lines of code / number of requested changes ever 😆
I will try to do better next time !
Thanks everyone for your patience.

…s to the github PR page

mossheim

thanks, in the future don't need the entire URL, just the number

changes addressed

fix Score examples and Platform.defaultTempDir on OSX

742b447

patrickdupuis previously requested changes Jan 4, 2019

View reviewed changes

gusano requested changes Jan 4, 2019

View reviewed changes

Change Platform.defaultTempDir on OSX platform only, revert back to /…

fe0a60e

…tmp for Linux

gusano requested changes Jan 4, 2019

View reviewed changes

add trailing slash to default temp dir on osx

e32175d

gusano approved these changes Jan 5, 2019

View reviewed changes

mossheim added comp: class library SC class library comp: help schelp documentation labels Jan 8, 2019

mossheim suggested changes Jan 16, 2019

View reviewed changes

fix if condition style and use +/+ instead of ++ in path concatenation

08f2500

mossheim reviewed Jan 17, 2019

View reviewed changes

Explain the trailing slash is due to backwards compatibility and link…

7507a86

…s to the github PR page

mossheim approved these changes Jan 17, 2019

View reviewed changes

mossheim merged commit bf30889 into supercollider:develop Jan 17, 2019

joshpar mentioned this pull request Jul 16, 2019

Fix build with boost-1.70 #4491

Closed

This was referenced Nov 11, 2020

OSX uses an SC managed defaultTempDir but never cleans it up #5239

Open

class library: don't warn by default when sending SynthDef #5238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix Score examples and Platform.defaultTempDir on OSX #4221

fix Score examples and Platform.defaultTempDir on OSX #4221

geoffroymontel commented Dec 21, 2018

patrickdupuis Jan 3, 2019

patrickdupuis Jan 4, 2019

gusano left a comment

geoffroymontel commented Jan 4, 2019

gusano Jan 4, 2019

geoffroymontel Jan 4, 2019

mossheim Jan 5, 2019

gusano Jan 5, 2019

geoffroymontel Jan 5, 2019

mossheim Jan 5, 2019

gusano left a comment

mossheim Jan 16, 2019

geoffroymontel Jan 16, 2019

geoffroymontel Jan 16, 2019

patrickdupuis Jan 16, 2019

geoffroymontel Jan 16, 2019

geoffroymontel Jan 16, 2019

geoffroymontel Jan 16, 2019

mossheim Jan 16, 2019

geoffroymontel Jan 16, 2019

geoffroymontel Jan 16, 2019

mossheim Jan 16, 2019

geoffroymontel Jan 16, 2019

alln4tural commented Jan 16, 2019 via email •

edited

Loading

geoffroymontel commented Jan 16, 2019

alln4tural commented Jan 16, 2019 via email

mossheim Jan 17, 2019

geoffroymontel Jan 17, 2019

mossheim left a comment

fix Score examples and Platform.defaultTempDir on OSX #4221

fix Score examples and Platform.defaultTempDir on OSX #4221

Conversation

geoffroymontel commented Dec 21, 2018

Purpose and Motivation

Types of changes

Checklist

Remaining Work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusano left a comment

Choose a reason for hiding this comment

geoffroymontel commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alln4tural commented Jan 16, 2019 via email • edited Loading

geoffroymontel commented Jan 16, 2019

alln4tural commented Jan 16, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossheim left a comment

Choose a reason for hiding this comment

alln4tural commented Jan 16, 2019 via email •

edited

Loading