Created attachment 170628 [details]
The devel/godot-tools port in shar format

Created attachment 170629 [details]
Proposed patch (since 415742 revision, second variant)

As you may know, there is a restriction to use target=release, when tools enabled. But there is a possibility to use target=release_debug, therefore I created a second variant for it, if you interested.

- Add RELEASE_DEBUG option to build with using release_debug target

Created attachment 170630 [details]
The poudriere testport log (FreeBSD 10.2 amd64, with default options): devel/godot

Created attachment 170631 [details]
The poudriere testport log (FreeBSD 10.2 amd64, with default options): devel/godot-tools

Created attachment 170632 [details]
The poudriere testport log (FreeBSD 10.2 amd64, with PULSEAUDIO enabled): devel/godot

Created attachment 170633 [details]
The poudriere testport log (FreeBSD 10.2 amd64, with PULSEAUDIO enabled): devel/godot-tools

With the license, it seems adding LICENSE_FILE is right and adding CC for the icon I missed is good. I would suggest you consider submitting a patch to update Mk/bsd.license.db.mk with the CC variations, maybe with a patch that also updates the various ports that currently have their own CC info.

The audio options is a good idea, though I'm not sure it's complete yet. I would choose rtaudio as the default as I find it more reliable than alsa. I should look into the audio more, but building godot 2.0 I found the default audio failed. It turned out that alsa was sending audio directly to /dev/dsp0 while my speakers are attached to dsp2, going through rtaudio gets the output right.

Using rtaudio isn't exclusive, it needs the alsa or pulse libs to work. While enabling rtaudio and using alsa or pulse works for me, godot talking straight to alsa failed. As rtaudio is a few files included with godot it could stay enabled rather than patching the other audio code. rtaudio also knows about LINUX OSS so if we get that working we could drop using audio libs from other ports and just use the built-in system sound. For now I think leave rtaudio enabled but choose between alsa and pulse using an OPTION_SINGLE.

Yes I did forget to install the icon, I think both the svg and png should be installed - and always, not just with tools enabled.

Using release_debug is worthwhile, I'm even thinking we should only use release or release_debug based on the tools selection. Maybe for anyone wanting a full debug version we could have a note saying add GODOT_DEBUG=yes in your make.conf and that will use debug instead of release_debug.

As for the extra tools port - I don't think there is a need for that. The build with the tools enabled works as a runtime. Although I do see a build without tools being useful, at this stage we don't have the games available for people to use a runtime only build. I had thought of removing tools from default once there are some games available, for now anyone installing godot is installing to use the tools to make a game.

Created attachment 170655 [details]
Proposed patch (since 415742 revision)

Hello.

(In reply to comment #7)
> I would suggest you consider submitting a patch to update
> Mk/bsd.license.db.mk with the CC variations, maybe with a patch that also
> updates the various ports that currently have their own CC info.
Thanks for suggestion. But, I don't consider this as a priority, currently.
There are various CC (Creative Commons) licenses and their versions, while I use just a portion of them for some ports, which I maintain.

(In reply to comment #7)
> Yes I did forget to install the icon, I think both the svg and png should be
> installed - and always, not just with tools enabled.
I think, there is no point to install icon(s), if TOOLS option disabled. The icon.png icon is needed for desktop entry only. Not sure where to install icon.svg. It could be ${PREFIX}/share/icons/hicolor/scalable/apps/godot.svg, but I didn't find a usage of it by desktop environment.
Moreover, if icon(s) will be installed independently from TOOLS option, then it will block proposed godot-tools port, because of file conflicts.

(In reply to comment #7)
> Using rtaudio isn't exclusive, it needs the alsa or pulse libs to work. While
> enabling rtaudio and using alsa or pulse works for me, godot talking straight
> to alsa failed.
Actually, it's possible to use RtAudio without ALSA or PulseAudio, then it will be called as RtAudio-None, so programs, which use audio, could be started (without audio).

(In reply to comment #7)
> As rtaudio is a few files included with godot it could stay enabled
I agree, it could be enabled by default, without option.

(In reply to comment #7)
> rtaudio also knows about LINUX OSS so if we get that working we could drop
> using audio libs from other ports and just use the built-in system sound.

I tried to build with "__LINUX_OSS__" define, but it gave following error:
drivers/rtaudio/RtAudio.cpp:8700:15: error: use of undeclared identifier 'AFMT_FLOAT'
  if ( mask & AFMT_FLOAT )

Also, it uses "/dev/mixer" in this case, which I didn't find on FreeBSD system.

(In reply to comment #7)
> For now I think leave rtaudio enabled but choose between alsa and pulse
> using an OPTION_SINGLE.
Actually, it is possible to disable these options, then it will be RtAudio-None. Therefore, this is just a OPTION_GROUP.
Also, ALSA and PULSEAUDIO options may be enabled together. I recreated patches for RtAudio driver, which allows to enumerate between them (look RtAudio::RtAudio(RtAudio::Api api) constructor on https://github.com/godotengine/godot/blob/2.0.3-stable/drivers/rtaudio/RtAudio.cpp#L197). But this could name a RtAudio-ALSA-PulseAudio driver in case of ALSA and PULSEAUDIO enabled together (ALSA checked first, then PULSEAUDIO). Still not sure, if this is practical (and better to use previous variant).

(In reply to comment #7)
> As for the extra tools port - I don't think there is a need for that. The
> build with the tools enabled works as a runtime. Although I do see a build
> without tools being useful, at this stage we don't have the games available
> for people to use a runtime only build. I had thought of removing tools from
> default once there are some games available, for now anyone installing godot
> is installing to use the tools to make a game.
There is a games/tanks-of-freedom port available, which may use just a runtime, e.g. with target=release, instead of target=debug, which tools=yes provide, currently. In practice, I even found how to build runtime and tools in the same godot port (with using do-build-TOOLS-on stage and modified DO_MAKE_BUILD command), because of different suffixes they use. But considering the users, which may use packages only, the godot and godot-tools ports are complete (runtime and tools) with default options. The important part is a SYMLINKSUFFIX variable, which godot-tools use, to not conflict between ports. But if you think otherwise, I can remove it from proposal, then it will be as current variant (without a multiple choice on the same computer).

Thanks for your attention.

Created attachment 170656 [details]
Proposed patch (since 415742 revision, second variant)

(In reply to comment #7)
> Using release_debug is worthwhile, I'm even thinking we should only use release
> or release_debug based on the tools selection. Maybe for anyone wanting a full
> debug version we could have a note saying add GODOT_DEBUG=yes in your make.conf
> and that will use debug instead of release_debug.
Let's agree on the first part, then propose additions to it, because your proposal requires more testing (and more TOOLSUF variants, I guess).

Created attachment 170661 [details]
Proposed patch (since 415742 revision, third variant)

(In reply to comment #7)
> As for the extra tools port - I don't think there is a need for that. The
> build with the tools enabled works as a runtime.
After some thinking about this, there is a possibility to use TOOLS option by default for devel/godot port and create devel/godot-runtime (or with different name, e.g. godot-run) with TOOLS option disabled. But need to agree about SYMLINKSUFFIX to use by dependent port(s) (e.g. it may be a godot-runtime or godot-run).
Still, this may create a conflict with devel/godot-runtime port, if user decides to disable TOOLS option for devel/godot port and then trying to install games/tanks-of-freedom, for example (while it is legit to run it either with godot or godot-runtime).

I will add this variant, just for example.

P.S.:
Looks like, we need to add gl to USE_GL, as new stage-qa suggested (USE_GL+=gl).
Sorry about so many variants.

Created attachment 170662 [details]
The devel/godot-runtime port in shar format (for third variant)

I removed current maintainer-approval requests, so you can choose concrete patch or propose your own, for example, based on parts of different variants.
I think, the proposed variants are possible solutions, based on different points of view, but need to choose something.
The base part: licenses, small fixes, ability to choose ALSA and/or PulseAudio drivers, desktop entry and icon(s) for TOOLS option.
Additional part: godot-tools or godot-runtime ports or nothing, release_debug only or also debug targets for options.
Need to choose which patches to use for RtAudio driver.

What is your opinion?

Created attachment 170708 [details]
Proposed patch (since 415742 revision)

Ok, based on your comment #7, I created "optimal" variant with following changes:
- Bump PORTREVISION
- Add LICENSE_FILE for MIT license
- Add CCBYv3 license for logo
- Add gl to USE_GL
- Add DEBUG option and new TOOLSUF variants
- Build TOOLS with release_debug target, when DEBUG option disabled (or WITH_DEBUG undefined)
- Allow to build ALSA and PulseAudio drivers independently
- Allow to build RtAudio driver with PulseAudio, in case of ALSA option is not selected
- Add desktop entry for TOOLS option
- Remove pkg-plist and use dynamic package list with PORTDATA and PLIST_FILES
- Silence mkdir

This time, I don't propose external port (e.g. godot-runtime). It could be easily created with adding SYMLINKSUFFIX (or other) variable and small changes, when needed.

Created attachment 170709 [details]
The poudriere testport log (FreeBSD 10.2 amd64, with default options)

Created attachment 170710 [details]
The poudriere testport log (FreeBSD 10.2 amd64, with PULSEAUDIO=on)

Created attachment 170711 [details]
The poudriere testport log (FreeBSD 10.2 amd64, with DEBUG=on TOOLS=off)

Created attachment 170758 [details]
improvements to devel/godot

A bit of a variation from your patch. Splitting the port into runtime and tools isn't that bad of an idea, I prefer godot and godot-tools over godot and godot-runtime.

I removed the tools option, if the slave port is being built then tools are enabled otherwise only the runtime is built. Make examples an option for godot-tools so that godot only installs a runtime for games to use as a dependency.

Added patch to get OSS working through rtaudio. This allows godot to run with the sound system in the base OS install.

I think we can remove the installing of the cryptic binary name and just install bin/godot and bin/godot-tools

Created attachment 170759 [details]
new port deve/godot-tools

shar for new port

(In reply to comment #7)
> A bit of a variation from your patch. Splitting the port into runtime and
> tools isn't that bad of an idea, I prefer godot and godot-tools over godot
> and godot-runtime.
Ok. I will comment about your proposed changes after some tests.
At first glance, I would still add DEBUG option, because it defines WITH_DEBUG automatically.

Created attachment 170766 [details]
Proposed patch (since 415742 revision)

(In reply to comment #17)
> Added patch to get OSS working through rtaudio.
> This allows godot to run with the sound system in the base OS install.
Good. It works.

(In reply to comment #17)
> I think we can remove the installing of the cryptic binary name and just
> install bin/godot and bin/godot-tools
I think, the BINSUFFIX variable is not needed in this case.

With some changes, the description may look like this:
- Bump PORTREVISION
- Add LICENSE_FILE for MIT license
- Add CCBYv3 license for logo
- Add gl to USE_GL
- Add DEBUG option
- Replace TOOLS option with devel/godot-tools port, while main port used as runtime
- Allow to build ALSA and PulseAudio drivers independently
- Allow to build RtAudio driver with OSS or ALSA or PulseAudio
- Set OSS option by default
- Add desktop entry
- Silence mkdir
- Remove pkg-plist and use dynamic package list with PORTDATA and PLIST_FILES

Created attachment 170767 [details]
The devel/godot-tools port in shar format

(In reply to comment #18)
> shar for new port
I moved EXAMPLES option to master port.
I think, the "Created by" line could be named for maintainer.

Created attachment 170772 [details]
Proposed patch (since 415742 revision)

Returned your define checks, because PKGNAMESUFFIX may be used for other purposes.

Created attachment 170773 [details]
The devel/godot-tools port in shar format

Also returned OPTIONS_DEFINE=EXAMPLES for devel/godot-tools port.
The OPTIONS_DEFAULT+=EXAMPLES not needed, as stated in a ports r415742.

Looks OK to me - see comment #20 for commit notes

(In reply to comment #24)
> Looks OK to me - see comment #20 for commit notes
Could you also approve the maintainer-feedback (on top right of the PR), for completion?

Thanks for your reviews and patches.

Created attachment 170803 [details]
The poudriere testport log (FreeBSD 10.2 amd64): devel/godot

Created attachment 170804 [details]
The poudriere testport log (FreeBSD 10.2 amd64): devel/godot-tools

Bump PR on FreeBSD ports mailing list.
The PR is complete. Assignee required.

Created attachment 171800 [details]
Proposed patch (since 415742 revision)

Added description for AUDIO options group, just in case.

Created attachment 171811 [details]
Proposed patch (since 415742 revision)

Removed mkdir, because of ${COPYTREE_SHARE} usage, which recreates destination directories.
Removed RM command for demos/2d/hexamap/.fscache file, because the file is not available for current version.

The description of changes may look as follows:
- Bump PORTREVISION
- Add LICENSE_FILE for MIT license
- Add CCBYv3 license for logo
- Add gl to USE_GL
- Add DEBUG option
- Replace TOOLS option with devel/godot-tools port, while main port used as runtime
- Allow to build ALSA and PulseAudio drivers independently
- Allow to build RtAudio driver with OSS or ALSA or PulseAudio
- Set OSS option by default
- Add desktop entry
- Remove mkdir, because of COPYTREE_SHARE usage, which recreates destination directories
- Remove RM command for demos/2d/hexamap/.fscache file, because the file is not available for current version
- Remove pkg-plist and use dynamic package list with PORTDATA and PLIST_FILES

Created attachment 171968 [details]
Proposed patch (since 415742 revision)

The USE_OPENSSL was deprecated in ports r417651. Added ssl to USES instead.

- Add ssl to USES, instead of deprecated USE_OPENSSL=yes

Created attachment 171969 [details]
The poudriere testport log (FreeBSD 10.2 amd64): devel/godot

(In reply to comment #31)
> The USE_OPENSSL was deprecated in ports r417651.
Looks like, the USE_OPENSSL wasn't deprecated in ports r417651, but replaced by USES+=ssl. Therefore, more correct description for change is follows:

- Replace USE_OPENSSL=yes with USES+=ssl

(In reply to comment #33)
> Looks like, the USE_OPENSSL wasn't deprecated
Or it was deprecated, because of following suggestion after make command:
"USE_OPENSSL is deprecated, please use USES=ssl"

Either way, I proposed a fix for it.

Comment 35 Kubilay Kocak 2016-06-30 06:57:08 UTC

I thought I'd drop in to say great job to you both on collaborating on this issue and using the tracker features very well. Keep it up! :)

Tips:

The issue summary should always reflect the overall changes, particularly if that changes over time. The issue currently only mentions one port, and not what the overall changes are. Something like:

"[NEW PORT] devel/godot-tools: ${COMMENT}, Improve devel/godot"

In most cases, its preferred and recommended to separate new port additions from updates. The next best thing is separate attachments, which has been done here.

Descriptions should include category/port so its clear what is for what (attachment 171968 [details]). A good rule of thumb to use is imagine its a commit message shortlog

Poudriere logs are no longer required to be included as attachments. Instead, just write the following in your description/comments after confirming they PASS:

- portlint: OK (looks fine.)
- testport: OK (poudriere: <archs>, <versions> tested)

Hello Kubilay Kocak.

(In reply to comment #35)
> I thought I'd drop in to say great job to you both on collaborating on this
> issue and using the tracker features very well. Keep it up! :)
Thanks :)

(In reply to comment #35)
> In most cases, its preferred and recommended to separate new port additions
> from updates.
This is true, but devel/godot-tools depends from devel/godot master port and developed in such a way, that some parts activated in devel/godot port, while other in devel/godot-tools (like EXAMPLES option). Therefore, I think, it's better to use this PR to not lose context. I did the same in bug 198088.

(In reply to comment #35)
> Poudriere logs are no longer required to be included as attachments.
Yes, this was suggested for me by other committer(s) some time ago. I added poudriere log for maintainer's review, because I found it useful, after ports changes and because I used them in PR creation process.

(In reply to comment #35)
> Instead, just write the following in your description/comments after
> confirming they PASS
Ok, I understood.

(In reply to comment #35)
> The issue summary should always reflect the overall changes, particularly if
> that changes over time. The issue currently only mentions one port, and not
> what the overall changes are.

Ok, I changed it.
Thanks for your attention.

To (possible) committer:
Please also mention maintainer in "Submitted by:" line, because the proposed changes in this PR were developed in collaboration.

Created attachment 172012 [details]
Proposed patch (since 415742 revision)

In private discussion, the maintainer reported about issue(s), related to error inside of OS_Unix::execute function (in ${WRKSRC}/drivers/unix/os_unix.cpp file), when using demos/gui/rich_text_bbcode project. I investigated this issue and suggested solution for it in a patch form.

Current (clarified) description of changes may look as follows:
- Bump PORTREVISION
- Add LICENSE_FILE for MIT license
- Add CCBYv3 license for logo
- Add devel/xdg-user-dirs, devel/xdg-utils and x11/xmessage to RUN_DEPENDS
- Replace USE_OPENSSL=yes with USES+=ssl
- Add gl to USE_GL
- Add DEBUG option and clarify build targets
- Replace TOOLS option with devel/godot-tools port, while devel/godot port used as runtime
- Allow to build ALSA and PulseAudio drivers independently
- Allow to build RtAudio driver with OSS or ALSA or PulseAudio
- Set OSS option by default
- Add desktop entry
- Add sed patch to fix executable paths inside of ${WRKSRC}/platform/x11/os_x11.cpp file
- Remove mkdir, because of COPYTREE_SHARE usage, which recreates destination directories
- Remove RM command for demos/2d/hexamap/.fscache file, because the file is not available for current version
- Add patch to enable OSS usage for RtAudio driver on FreeBSD
- Add patch to fix issue(s) with OS_Unix::execute and OS_Unix::get_executable_path functions on FreeBSD
- Remove pkg-plist and use dynamic package list with PORTDATA and PLIST_FILES

Created attachment 172034 [details]
Proposed patch (since 415742 revision)

Added fallback code for OS_Unix::get_executable_path function, similar to Linux variant.

Yes the patch for drivers/unix/os_unix.cpp looks to be a working solution. Thanks.

I was going to suggest the same parse_utf8() change.

I don't see the need for the extra if() - To include the extra WARN_PRINT add it after if(sysctl()!=0) when you set the string to zero length, as you setting the string to zero length there would be no point in creating the String and testing if it equals ""

Created attachment 172038 [details]
Proposed patch (since 415742 revision)

(In reply to comment #41)
> I don't see the need for the extra if() - To include the extra WARN_PRINT
> add it after if(sysctl()!=0) when you set the string to zero length, as you
> setting the string to zero length there would be no point in creating the
> String and testing if it equals ""
I guess, the check for empty string might be needed, in case sysctl returns empty string (not sure, if this is possible, but still), like in Linux case for readlink. This is two exception cases, which were combined into one WARN_PRINT. The "buf[0] = '\0';" is similar to Linux case with "memset(buf,0,256);"

But from the code of vn_fullpath1 function, it assigns '\0' to last buf(fer) position:
https://github.com/freebsd/freebsd/blob/28823d06561e2e9911180b17a57e05ff19d7cbf6/sys/kern/vfs_cache.c#L1303
So it should be ok without manual assign.

You could check various examples on the following search request:
https://github.com/freebsd/freebsd/search?q=KERN_PROC_PATHNAME

The source code for sysctl_kern_proc_pathname function is here, for reference:
https://github.com/freebsd/freebsd/blob/736e07849585def123cc8c3042c05c77f3d11e2e/sys/kern/kern_proc.c#L1995

Particularly, there is even "/proc/curproc/file" example (for readlink usage, instead of "/proc/self/exe", like in Linux case):
https://github.com/freebsd/freebsd/blob/bb4ff000de429abf7115b8d078833c468387f1dd/contrib/libexecinfo/backtrace.c#L55
which works, but returns warnings several times, if used similarly as for Linux case.

But there is explicit differentiation in compiler-rt source code (sysctl for FreeBSD and readlink for others):
https://github.com/freebsd/freebsd/blob/9c27ec33f2c82fe6c60c9c375a88f96a1e10a6a2/contrib/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc#L715

In case of pathname request for other process, there is an example with check for "len == 0 || strlen(pathname) == 0" and "No cached pathname for process" error:
https://github.com/freebsd/freebsd/blob/290f0204ae95d6dc92ba9c15fafe090e9d4338af/tools/test/ptrace/scescx.c#L170

New patch attached, with your suggestion.

Created attachment 172041 [details]
Proposed patch (since 415742 revision)

Fixed "find" function usage inside of "OS_Unix::execute" function.

(In reply to lightside from comment #42)

The proc filesystem is optional on FreeBSD, so we can't rely on it being there. If it is there then the /proc path could be used, which would mean checking if it is there and if it isn't, then use sysctl, as the sysctl call has to be there, there is no point of the extra check of whether /proc exists.

http://freebsd.1045724.n5.nabble.com/Why-is-procfs-deprecated-in-favor-of-procstat-td4028960.html
https://www.freebsd.org/doc/en/articles/linux-users/procfs.html

The libexecinfo/backtrace.c example sets the path, if KERN_PROC_PATHNAME exists then it overrides the earlier /proc path definition by calling sysctl.

The ptrace example is expected to be asking for paths to other executables with many possible outcomes, any situation could arise from that, no permission, process crashes before the path was returned.... so extra checks can be expected.


(In reply to lightside from comment #43)

I don't think my earlier change to OS_Unix::execute() should be added back. It was short sighted of me to do that change when I first got godot running, at that time I didn't see it being used outside of initially starting godot. While I think using execvp() is useful, the use of getprogname() would have to be removed. As this change would make FreeBSD act differently to other platforms I don't think leaving it there is a good idea. Let's keep the FreeBSD specific code limited to OS_Unix::get_executable_path()

Comment on attachment 172038 [details]
Proposed patch (since 415742 revision)

(In reply comment #44)
> I don't think my earlier change to OS_Unix::execute() should be added back.
Ok, I returned attachment #172038 [details], with previous approach.

Created attachment 172374 [details]
Proposed patch (since 415742 revision)

There is new 2.0.4.1 stable version available:
http://downloads.tuxfamily.org/godotengine/2.0.4.1/Godot_v2.0.4.1_stable_changelog.txt
https://github.com/godotengine/godot/compare/2.0.3-stable...2.0.4.1-stable
https://godotengine.org/article/maintenance-release-godot-2-0-4

I changed summary of the PR. Maybe update will get more attention.

Tested with using poudriere on FreeBSD 9.3 amd64 and native build on FreeBSD 10.2 amd64.
The correct value of TIMESTAMP, based on greatest timestamp of modified files, was produced with following command:
% make clean extract && find work/* -type f -print0 | xargs -0 stat -f '%m' | sort -u | tail -1

Created attachment 172548 [details]
update for devel/godot

With the upcoming release of 11.0 we also need to add a small adjustment to RtAudio.cpp.

This updated patch adds the following to the previous 2.0.4.1 update to the end of files/patch-drivers_rtaudio_RtAudio.cpp. sampleRate is an unsigned int so srate gets promoted to unsigned int, this means the result is always positive so the use of abs() is of no use.

@@ -9025,7 +9043,7 @@ bool RtApiOss :: probeDeviceOpen( unsign
   }
 
   // Verify the sample rate setup worked.
-  if ( abs( srate - sampleRate ) > 100 ) {
+  if ( ( srate - sampleRate ) > 100 ) {
     close( fd );
     errorStream_ << "RtApiOss::probeDeviceOpen: device (" << ainfo.name << ") does not support sample rate (" << sampleRate << ").";
     errorText_ = errorStream_.str();

Comment on attachment 172374 [details]
Proposed patch (since 415742 revision)

adjustment for 11.0 should be added

(In reply to comment #49)
> With the upcoming release of 11.0 we also need to add a small adjustment to RtAudio.cpp.
>
> This updated patch adds the following to the previous 2.0.4.1 update to the
> end of files/patch-drivers_rtaudio_RtAudio.cpp. sampleRate is an unsigned int
> so srate gets promoted to unsigned int, this means the result is always
> positive so the use of abs() is of no use.

I think, that your logic about srate is wrong, because srate defined as int:
https://github.com/godotengine/godot/blob/820dd1d0016763cda6177104e66e09c8634150be/drivers/rtaudio/RtAudio.cpp#L9018
I placed following debug code near your patched place:
printf("srate=%d, sampleRate=%d, srate - sampleRate = %d, abs(srate - sampleRate) = %d\n", srate, sampleRate, srate - sampleRate, abs(srate - sampleRate));
with following output:
srate=44100, sampleRate=44100, srate - sampleRate = 0, abs(srate - sampleRate) = 0

But there are other possible cases, where abs is needed.
-8<--
#include <stdio.h>
#include <stdlib.h>

int check(int a, unsigned int b)
{
	return (a - b) > 100;
}

int check_abs(int a, unsigned int b)
{
	return abs(a - b) > 100;
}

int main()
{
	unsigned int c = 44200;
	for (int i = 44100; i <= 44300; i += 100)
		printf("check(%d, %d) = %d; check_abs(%d, %d) = %d\n", i, c, check(i, c), i, c, check_abs(i, c));

	return 0;
}
-->8-
% c++ test.cpp -o test && ./test
check(44100, 44200) = 1; check_abs(44100, 44200) = 0
check(44200, 44200) = 0; check_abs(44200, 44200) = 0
check(44300, 44200) = 0; check_abs(44300, 44200) = 0

Without abs, the first case is wrong, i.e. possible srate = 44100 and sampleRate = 44200.
There are cases with fabs in the same code:
https://github.com/godotengine/godot/blob/820dd1d0016763cda6177104e66e09c8634150be/drivers/rtaudio/RtAudio.cpp#L1093
https://github.com/godotengine/godot/blob/820dd1d0016763cda6177104e66e09c8634150be/drivers/rtaudio/RtAudio.cpp#L1148

And this is not related to FreeBSD 11 at all, because issue with abs is different, related to possible abs redefinition. There are many other places in the source code with abs usage:
https://github.com/godotengine/godot/blob/820dd1d0016763cda6177104e66e09c8634150be/drivers/opus/celt/celt_encoder.c#L651
https://github.com/godotengine/godot/blob/820dd1d0016763cda6177104e66e09c8634150be/drivers/opus/celt/cwrs.c#L447
https://github.com/godotengine/godot/blob/820dd1d0016763cda6177104e66e09c8634150be/drivers/theora/analyze.c#L753
https://github.com/godotengine/godot/blob/820dd1d0016763cda6177104e66e09c8634150be/drivers/vorbis/floor1.c#L265
etc.

(In reply to comment #51)
> There are cases with fabs in the same code:
> https://github.com/godotengine/godot/blob/820dd1d0016763cda6177104e66e09c8634150be/drivers/rtaudio/RtAudio.cpp#L1093
> https://github.com/godotengine/godot/blob/820dd1d0016763cda6177104e66e09c8634150be/drivers/rtaudio/RtAudio.cpp#L1148
but it related to __MACOSX_CORE__.

I found the cause of your proposed changes, if compiling the test.cpp code (from comment #51) with Clang 3.8 compiler from devel/llvm38 port:
-8<--
% clang++38 test.cpp -o test_38
test.cpp:11:9: warning: taking the absolute value of unsigned type 'unsigned int' has no effect [-Wabsolute-value]
        return abs(a - b) > 100;
               ^
test.cpp:11:9: note: remove the call to 'abs' since unsigned values cannot be negative
        return abs(a - b) > 100;
               ^~~
1 warning generated.
-->8-
But it gave the same wrong result without abs:
-8<--
% ./test_38 
check(44100, 44200) = 1; check_abs(44100, 44200) = 0
check(44200, 44200) = 0; check_abs(44200, 44200) = 0
check(44300, 44200) = 0; check_abs(44300, 44200) = 0
-->8-
Therefore, this Clang 3.8 compiler's suggestion is wrong also.

Created attachment 172566 [details]
Proposed patch (since 415742 revision)

Even GCC 4.8.5 from current lang/gcc port doesn't give such suggestions as Clang 3.8:
-8<--
% g++48 -Wall -Wextra test.cpp -o test_g++48 && ./test_g++48
check(44100, 44200) = 1; check_abs(44100, 44200) = 0
check(44200, 44200) = 0; check_abs(44200, 44200) = 0
check(44300, 44200) = 0; check_abs(44300, 44200) = 0
-->8-
But GCC 6.1.0 from current lang/gcc6 port gives following error:
-8<--
% g++6 -Wall -Wextra test.cpp -o test_g++6 && ./test_g++6
test.cpp: In function 'int check_abs(int, unsigned int)':
test.cpp:11:18: error: call of overloaded 'abs(unsigned int)' is ambiguous
  return abs(a - b) > 100;
                  ^
In file included from /usr/local/lib/gcc6/include/c++/cstdlib:75:0,
                 from /usr/local/lib/gcc6/include/c++/stdlib.h:36,
                 from test.cpp:2:
/usr/local/lib/gcc6/gcc/x86_64-portbld-freebsd10.2/6.1.0/include-fixed/stdlib.h:100:6: note: candidate: int abs(int)
 int  abs(int) __pure2;
      ^~~
In file included from /usr/local/lib/gcc6/include/c++/stdlib.h:36:0,
                 from test.cpp:2:
/usr/local/lib/gcc6/include/c++/cstdlib:185:3: note: candidate: __int128 std::abs(__int128)
   abs(__GLIBCXX_TYPE_INT_N_0 __x) { return __x >= 0 ? __x : -__x; }
   ^~~
/usr/local/lib/gcc6/include/c++/cstdlib:180:3: note: candidate: long long int std::abs(long long int)
   abs(long long __x) { return __builtin_llabs (__x); }
   ^~~
/usr/local/lib/gcc6/include/c++/cstdlib:172:3: note: candidate: long int std::abs(long int)
   abs(long __i) { return __builtin_labs(__i); }
   ^~~
-->8-
Therefore, the fix might be "abs(a - (int)b) > 100":
-8<--
#include <stdio.h>
#include <stdlib.h>

int check(int a, unsigned int b)
{
	return (a - b) > 100;
}

int check_abs(int a, unsigned int b)
{
	return abs(a - (int)b) > 100;
}

int main()
{
	unsigned int c = 44200;
	for (int i = 44100; i <= 44300; i += 100)
		printf("check(%d, %d) = %d; check_abs(%d, %d) = %d\n", i, c, check(i, c), i, c, check_abs(i, c));

	return 0;
}
-->8-
-8<--
% g++6 -Wall -Wextra test2.cpp -o test2_g++6 && ./test2_g++6
check(44100, 44200) = 1; check_abs(44100, 44200) = 0
check(44200, 44200) = 0; check_abs(44200, 44200) = 0
check(44300, 44200) = 0; check_abs(44300, 44200) = 0
-->8-

while the implementation of "-Wabsolute-value" in Clang (3.6, 3.7 and) 3.8 is questionable (about exact suggestions).

On 10.3 clang 3.8 from ports you get a warning,
On 11.0 clang 3.8 in base it is an error that stops the build.
So it is changes in the system headers/libs in 11.0 that leaves abs(unsigned int) as no longer accepted, not clang 3.8 itself, unless 11.0 has different warning defaults compiled in compared to ports clang, I don't get the info about the -Wabsolute-value flag in 11.0 so there is a variation somewhere.

In the failing piece of code sampleRate is an unsigned int which is why this one fails where the other uses of abs and fabs compile, as they use int and float with some casts to suit. We could also add a cast here (abs(srate-(int)sampleRate)>100) to get it to compile, which would match with other existing code. I chose removing abs as the use of unsigned int should remove the need.

To me the one odd result you get in your test shouldn't be returning true. That odd result is saying that (44100-44200) is greater than 100 - by itself (44100-44200) will return -100 then comparing that to 100 it is saying that -100 is greater than 100, even dropping the sign they should be equal.


The error on 11.0 -

drivers/rtaudio/RtAudio.cpp:9046:8: error: call to 'abs' is ambiguous
  if ( abs( srate - sampleRate ) > 100 ) {
       ^~~
/usr/include/stdlib.h:83:6: note: candidate function
int      abs(int) __pure2;
         ^
/usr/include/c++/v1/stdlib.h:115:44: note: candidate function
inline _LIBCPP_INLINE_VISIBILITY long      abs(     long __x) _NOEXCEPT {return  labs(__x);}
                                           ^
/usr/include/c++/v1/stdlib.h:117:44: note: candidate function
inline _LIBCPP_INLINE_VISIBILITY long long abs(long long __x) _NOEXCEPT {return llabs(__x);}
                                           ^
/usr/include/c++/v1/math.h:646:1: note: candidate function
abs(float __lcpp_x) _NOEXCEPT {return fabsf(__lcpp_x);}
^
/usr/include/c++/v1/math.h:650:1: note: candidate function
abs(double __lcpp_x) _NOEXCEPT {return fabs(__lcpp_x);}
^
/usr/include/c++/v1/math.h:654:1: note: candidate function
abs(long double __lcpp_x) _NOEXCEPT {return fabsl(__lcpp_x);}
^
1 error generated.
scons: *** [drivers/rtaudio/RtAudio.x11.opt.64.llvm.o] Error 1
scons: building terminated because of errors.
===> Compilation failed unexpectedly.
Try to set MAKE_JOBS_UNSAFE=yes and rebuild before reporting the failure to
the maintainer.
*** Error code 1

Stop.
make[1]: stopped in /usr/ports/devel/godot
*** Error code 1

Stop.
make: stopped in /usr/ports/devel/godot

(In reply to comment #55)
> We could also add a cast here (abs(srate-(int)sampleRate)>100) to get it to
> compile, which would match with other existing code.
I did exactly this in attachment #172566 [details].

(In reply to comment #55)
> To me the one odd result you get in your test shouldn't be returning true.
This is how it works with unsigned int and without abs.

(In reply to comment #56)
> This is how it works with unsigned int and without abs.
-8<--
#include <stdio.h>
#include <stdlib.h>

int main()
{
	for (int i = 20; i <= 100; i+=20)
		printf("%d = %u; abs(%d) = %u\n", -i, -i, -i, abs(-i));

	return 0;
}
-->8-
% clang++38 out.cpp -o out && ./out
-20 = 4294967276; abs(-20) = 20
-40 = 4294967256; abs(-40) = 40
-60 = 4294967236; abs(-60) = 60
-80 = 4294967216; abs(-80) = 80
-100 = 4294967196; abs(-100) = 100

In other words, -100 in unsigned int is 4294967196, which is bigger than 100.

Created attachment 173522 [details]
Proposed patch (since 415742 revision)

There is new 2.1 stable version available:
http://downloads.tuxfamily.org/godotengine/2.1/Godot_v2.1-stable_changelog_authors.txt
https://github.com/godotengine/godot/compare/2.0.4.1-stable...2.1-stable
https://godotengine.org/article/godot-reaches-2-1-stable

Changes to previous version:
- Moved glu to build dependency.
- Added xrandr to USE_XORG.
- Moved godot demos to devel/godot-demo-projects port, as upstream did.
- Adapted patches.

Created attachment 173523 [details]
The devel/godot-demo-projects port in shar format

Godot demo projects.

Each folder containing an engine.cfg file is a demo project meant to be used
with Godot Engine, the open source 2D and 3D game engine.

WWW: https://github.com/godotengine/godot-demo-projects

Created attachment 173524 [details]
The devel/godot-tools port in shar format

Added runtime dependency on devel/godot-demo-projects for EXAMPLES option.

While moving glu to BUILD_DEPENDS works, it looks like it could be removed altogether. godot builds and the demos run with the include<GL/glu.h> removed. I have submitted an issue asking whether it is really needed.

rtaudio isn't working for me now, need to find why.

For demo projects, I think it better to have the PORTVERSION of 2.1 to match the release rather than a date. Instead of using a github download we could use the Godot-Demos-2.1-latest.zip available from the godotengine.org website.

The pkg-descr for demo projects could be better, I'm thinking -

Godot demo projects is a  collection of small projects to demonstrate
various features of the Godot game engine.

Open the engine.cfg files with godot-tools to see how to use each feature.

Created attachment 173710 [details]
Proposed patch (since 415742 revision)

(In reply to comment #61)
> While moving glu to BUILD_DEPENDS works, it looks like it could be removed
> altogether. godot builds and the demos run with the include<GL/glu.h> removed.
The "GL/glu.h" required in "drivers/gl_context/GL/glew.h" file at build time.
The Godot's build system includes instructions to build static version of GLEW in "drivers/gl_context/SCsub" file.

(In reply to comment #61)
> rtaudio isn't working for me now, need to find why.
The RtAudio-OSS works for me, e.g. for games/tanks-of-freedom.

(In reply to comment #61)
> Instead of using a github download we could use the Godot-Demos-2.1-latest.zip
> available from the godotengine.org website.
Thanks, I integrated it into distinfo file of devel/godot port instead.

Created attachment 173711 [details]
Proposed patch (since 415742 revision)

(In reply to comment #61)
> While moving glu to BUILD_DEPENDS works, it looks like it could be removed
> altogether.
Possible to disable "GL/glu.h" include with using CXXFLAGS+=-DGLEW_NO_GLU.

(In reply to comment #61)
> I have submitted an issue asking whether it is really needed.
Ok. I attached version of proposed patch without graphics/libGLU build dependency, just in case.

Created attachment 173719 [details]
Proposed patch (since 415742 revision)

Added variant with GH_TUPLE for Godot demos.
In this case, the port will use https://github.com/godotengine/godot-demo-projects repository and may require to specify concrete commit for new version.
The variant in attachment #173711 [details] may be easy to update (because of no available (the same) tag name for demos), but I'm not sure about Godot-Demos-2.1-latest.zip file (is it a link or just a regular (stable) file?), where there is other Godot-Demos-2.1-20160729.zip file available in the same download directory:
https://downloads.tuxfamily.org/godotengine/demos/2.1/

To Shane:
Please specify other available attachment, if you think about other variant, so I can return it, or propose your own version.

This is ready to be committed.

Description of changes:
- Update to 2.1 release
- Add LICENSE_FILE for MIT license
- Add CCBYv3 license for logo
- Add devel/xdg-user-dirs, devel/xdg-utils and x11/xmessage to RUN_DEPENDS
- Add xrandr to USE_XORG
- Replace USE_OPENSSL=yes with USES+=ssl
- Add gl to USE_GL
- Remove GLU dependency - glu.h was included from imported glew.h but GLU components are not used
- Add DEBUG option and clarify build targets
- Replace TOOLS option with devel/godot-tools port, while devel/godot port used as runtime
- Allow to build ALSA and PulseAudio drivers independently
- Allow to build RtAudio driver with OSS or ALSA or PulseAudio
- Set OSS option by default
- Add desktop entry for godot-tools
- Add sed patch to fix executable paths inside of ${WRKSRC}/platform/x11/os_x11.cpp file
- Remove mkdir, because of COPYTREE_SHARE usage, which recreates destination directories
- Remove RM command for demos/2d/hexamap/.fscache file, because the file is not available for current version
- Add patch to enable OSS usage for RtAudio driver on FreeBSD
- Add patch to fix issue(s) with OS_Unix::execute and OS_Unix::get_executable_path functions on FreeBSD
- Add patch to fix layout of --help text
- Remove pkg-plist and use dynamic package list with PORTDATA and PLIST_FILES
- Use current head commit of 2.1 branch from new github repo godotengine/godot-demo-projects to install examples (this was previously part of godot repo)

Comment 66 Kurt Jaeger 2016-08-21 08:07:17 UTC

testbuilds@work

Comment 67 Jan Beich 2016-08-21 09:06:39 UTC

Created attachment 173905 [details]
style cleanup

poudriere bulk -t is green for 9.3R-i386, 9.3R-amd64, 10.1R-i386, 10.3R-amd64, 11.0R-i386 here.

> USE_GITHUB=	yes
> GH_ACCOUNT=	godotengine
> +GH_TAGNAME_DEMOS=	dfa1274
[...]
> +EXAMPLES_GH_TUPLE=	godotengine:godot-demo-projects:${GH_TAGNAME_DEMOS}:DEMOS

GH_TAGNAME_DEMOS kind of defeats the simplicity of GH_TUPLE. "godotengine" can also be replaced with "${GH_ACCOUNT}". And if you have a rationale to choose a specific commit then better annotate the line via an inline comment.

> -USES=		scons pkgconfig compiler
> -USE_OPENSSL=	yes
> +USES=		scons ssl pkgconfig compiler

If you have the chance do sort USES value.

> +AUDIO_DESC=		Supported audio

<transitive verb> + <adjective>. Where's a noun? Maybe use "Audio support" or "Audio backends" instead.

> +GODOTFILE=		${PORTNAME}${PKGNAMESUFFIX}

GODOTFILE=${PKGBASE} would be shorter but I don't understand why you need an extra variable.

> +.if ${SLAVE_PORT} == yes

SLAVE_PORT conditionals are almost always an abomination and often can be replaced with ?= variables.

> .if ${CHOSEN_COMPILER_TYPE} == clang
> MAKE_ARGS+=	use_llvm=yes
> .else  # clang
> USE_GCC=	yes
> .if ${ARCH} == i386
> CXXFLAGS+=	-march=i586
> .endif
> .endif # clang

Another case of logic that can be easily moved above .include <bsd.port.pre.mk>.

> do-install-EXAMPLES-on:
> 	(cd ${WRKSRC_DEMOS} && ${COPYTREE_SHARE} "${PORTDATA}" \
> 		${STAGEDIR}${DATADIR}/demos)

This is confusing. Use PORTEXAMPLES when installing EXMAPLES.

> ++#if defined(__FreeBSD__)
> ++#define SND_DEVICE "/dev/dsp"
> ++#else
> ++#define SND_DEVICE "/dev/mixer"
> ++#endif

__DragonFly__ inherited __FreeBSD__ audio stack, so it may want the same.

> + #elif defined(__FreeBSD__)
[...]
> ++	int mib[4] = { CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1 };

Maybe replace with #elif defined(KERN_PROC_PATHNAME) which is available on FreeBSD, DragonFly and NetBSD.

(In reply to comment #67)
> GH_TAGNAME_DEMOS kind of defeats the simplicity of GH_TUPLE.
> "godotengine" can also be replaced with "${GH_ACCOUNT}"
The "Variable GH_ACCOUNT is recursive" in your proposed case. I tried this before.

(In reply to comment #67)
> GODOTFILE=${PKGBASE} would be shorter but I don't understand why you need
> an extra variable.
Because PKGBASE has different value: ${PKGNAMEPREFIX}${PORTNAME}${PKGNAMESUFFIX}
https://svnweb.freebsd.org/ports/head/Mk/bsd.port.mk?revision=420461&view=markup#l2572
This is just an variable, which not used in port's framework, i.e. to change it in one place, instead of many places, when needed.

(In reply to comment #67)
> SLAVE_PORT conditionals are almost always an abomination and often can be
> replaced with ?= variables.
This is how it works and proposed by maintainer. Possible to check ${PKGNAMESUFFIX:M-tools} or ${PKGBASE:Mgodot-tools} directly.

(In reply to comment #67)
> This is confusing. Use PORTEXAMPLES when installing EXMAPLES.
I think, The DEMOS option is more appropriate here, but this may add new value to OPTIONS_DEFAULT. The EXAMPLES option was more simple to use, I guess. Moreover, the demos files installed into ${DATADIR}/demos, not ${EXAMPLESDIR}. This is how it is in current (2.0.3) version and how user's paths may be configured already.

(In reply to comment #67)
> __DragonFly__ inherited __FreeBSD__ audio stack, so it may want the same.
The delta ports (for DragonFlyBSD) uses "alias" for devel/godot port:
https://svnweb.freebsd.org/ports/head/Mk/Uses/alias.mk?revision=391257&view=markup
https://github.com/DragonFlyBSD/DeltaPorts/blob/10b54b72b5524f8f4936daf80ae5d9a96a537e8c/ports/devel/godot/Makefile.DragonFly
https://github.com/DragonFlyBSD/DPorts/blob/6ae8d72bf6afc35d1c487d6b3ce56f56423f149f/devel/godot/Makefile.DragonFly
And this is not only place with __FreeBSD__ define:
https://github.com/godotengine/godot/search?q=__FreeBSD__

(In reply to comment #67)
> Maybe replace with #elif defined(KERN_PROC_PATHNAME) which is available on
> FreeBSD, DragonFly and NetBSD.
I think, this statement is for upstream, where __FreeBSD__ define already used here. See answer about "alias" above.

Comment 69 Kurt Jaeger 2016-08-21 15:44:47 UTC

Both variants testbuild fine. Now, which one can be committed ?

To Jan Beich:
I didn't check your attachment #173905 [details], when answered to comment #67.
Maybe maintainer has different opinion and/or proposed changes.

Thanks for review.

Created attachment 173912 [details]
Proposed patch (since 415742 revision)

(In reply to comment #67)
> If you have the chance do sort USES value.
(In reply to comment #67)
> <transitive verb> + <adjective>. Where's a noun? Maybe use "Audio support"
> or "Audio backends" instead.
Ok, proposed. Not sure about compiler:c++11-lib, currently:
% make extract && find work/* -type f | xargs grep -l -e "c++11" | wc -l
       0

(In reply to comment #68)
> I didn't check your attachment #173905 [details]
I checked attachment #173905 [details]. Among others, you proposed other method to do changes for devel/godot-tools port, where approved method is to do changes in devel/godot port instead, which may reduce possible changes for devel/godot-tools port.

(In reply to comment #69)
> Both variants testbuild fine.
Thanks.

(In reply to comment #69)
> Now, which one can be committed ?
There is a need for maintainer's opinion/approval and/or other proposed changes, I think.

Comment 73 Jan Beich 2016-08-21 17:25:13 UTC

Comment on attachment 173905 [details]
style cleanup

(In reply to Kurt Jaeger from comment #69)
> Both variants testbuild fine. Now, which one can be committed ?

Ignore my patch per "perfect is the enemy of good". Except for sorting USES and rewording AUDIO_DESC it's an aggressive approach to style that can go in a separate bug.

(In reply to lightside from comment #68)
> Because PKGBASE has different value: ${PKGNAMEPREFIX}${PORTNAME}${PKGNAMESUFFIX}

Which is more correct if devel/linux-godot-tools ever appears. ;)

> This is just an variable, which not used in port's framework, 

GODOTFILE isn't but PKGNAMESUFFIX is. If you're relying on parts of the package name then why not take advantage of it more.

> i.e. to change it in one place, instead of many places, when needed.

Before the update here lands GODOTFILE isn't defined in devel/godot. Dropping it now wouldn't affect |svn blame|.

> I think, this statement is for upstream...

Upstream doesn't use KERN_PROC_PATHNAME.

(In reply to comment #73)
> Upstream doesn't use KERN_PROC_PATHNAME.
The mentioned part ("#elif defined(__FreeBSD__)") was proposed/committed by maintainer of devel/godot port:
https://github.com/godotengine/godot/commit/73ca870c81145865d5a73c7aa95431c6792d5ec1#diff-b9a6881cce16c6f2d4c33e0f1acc0921R436
The current "alias" usage in delta ports extends this for DragonFlyBSD operating system, where KERN_PROC_PATHNAME is available:
https://github.com/DragonFlyBSD/DragonFlyBSD/blob/713fde5812833a2c64fb28e978e03228ca4cc59a/sys/sys/sysctl.h#L467
I agree, that possible patch for upstream might be more general, if it should support more operating systems.

Created attachment 173917 [details]
Proposed patch (since 415742 revision)

(In reply to comment #67)
> GH_TAGNAME_DEMOS kind of defeats the simplicity of GH_TUPLE.
Possible to propose your variant for this case.

Created attachment 173935 [details]
Proposed Makefile for devel/godot

This is a proposed Makefile (complete file for consideration) to be used in devel/godot

Using SLAVE_PORT was how I was told to do it in another port. Change to using PKGNAMESUFFIX which means defining it as empty and doesn't seem as clean to me.

Use PORTEXAMPLES/EXAMPLESDIR

For the patches I have used #if defines(__FreeeBSD__) to be clear that that is the only system the change was made for and that I know to be working. If after users on other systems build godot and need similar changes that can share the same code then the test can be expanded or a more general test can be put into place. I'm not familiar with the other bsd's to offer a more general test. Lightside - the other usages of __FreeBSD__ that exist are from what I pushed upstream after getting v1.0 working.

Created attachment 173937 [details]
Proposed patch (since 415742 revision)

(In reply to comment #76)
> Using SLAVE_PORT was how I was told to do it in another port.
This is right approach, in my opinion:
https://svnweb.freebsd.org/ports/head/Mk/bsd.port.mk?revision=420461&view=markup#l1187
Even x11-servers/xorg-server port uses SLAVE_PORT checks:
https://svnweb.freebsd.org/ports/head/x11-servers/xorg-server/Makefile?revision=416966&view=markup#l57
as many others:
https://github.com/freebsd/freebsd-ports/search?q=SLAVE_PORT

I agreed about SLAVE_PORT approach before, because value of PKGNAMESUFFIX variable may be changed to create a different package (with WITH_DEBUG=yes, for example):
% make PKGNAMESUFFIX=-tools-debug -V PKGBASE
godot-tools-debug

Therefore, the value of PKGNAMESUFFIX variable is not quite useful for checks here, in my opinion.

(In reply to comment #76)
> Use PORTEXAMPLES/EXAMPLESDIR
Ok, I changed PORTDATA and DATADIR to PORTEXAMPLES and EXAMPLESDIR. But this may invalidate current user's paths for ${DATADIR}/demos projects.

> PORTEXAMPLES=	2d 3d gui misc plugins viewport
The README.md (including a reference to LICENSE.md file) maybe useful, in my opinion, while manual list of directories (instead of just *) may require to check them again for new version(s).

> do-install-EXAMPLES-on:
> 	@${ECHO} 'installing examples...'
> .for d in ${PORTEXAMPLES}
> 	@(cd ${WRKSRC_XMPL} && ${COPYTREE_SHARE} ${d} \
> 		${STAGEDIR}${EXAMPLESDIR})
> .endfor
The ${COPYTREE_SHARE} is a shell script, where first argument may be a list of files/directories (i.e. "${PORTEXAMPLES}"):
https://svnweb.freebsd.org/ports/head/Mk/bsd.port.mk?revision=420461&view=markup#l550
https://svnweb.freebsd.org/ports/head/Mk/bsd.port.mk?revision=420461&view=markup#l2153
No need to use a "for" loop here, in my opinion.

> do-install:
> 	@cd ${WRKSRC}/bin && ${INSTALL_SCRIPT} godot.x11* \
> 		${STAGEDIR}/${PREFIX}/bin/${GODOTFILE}
> 	${STRIP_CMD} ${STAGEDIR}/${PREFIX}/bin/${GODOTFILE}
The current usage of INSTALL_PROGRAM is right and not requires to strip a binary.

(In reply to comment #76)
> If after users on other systems build godot and need similar changes that
> can share the same code then the test can be expanded or a more general test
> can be put into place.
I agree with this. The full support for DragonFlyBSD (or other operating system) may require to create a patches for other files, unrelated to this PR. This might be a (Git) pull request for upstream from experienced people, which could test these changes on DragonFlyBSD, for example. While "alias" usage in delta ports for devel/godot doesn't require to add patches for it (currently):
https://svnweb.freebsd.org/ports/head/Mk/Uses/alias.mk?revision=391257&view=markup#l4

Created attachment 173942 [details]
Proposed patch (since 415742 revision)

Sorted LICENSE_FILE_* variables.

Created attachment 173959 [details]
Proposed patch (since 415742 revision)

(In reply to comment #76)
> OPTIONS_DEFAULT=	OSS
I noticed, that you proposed to remove "+=". This is ok, if there are no other default option(s) used for devel/godot-tools port (The EXAMPLES option is not affected).

The removal of OPTIONS_DEFINE+=DEBUG (and related code) is not ok, because there is a target=debug option available, which maybe used for tools=no and tools=yes options:
https://github.com/godotengine/godot/blob/2.1-stable/SConstruct#L116
https://github.com/godotengine/godot/blob/2.1-stable/SConstruct#L181
https://github.com/godotengine/godot/blob/2.1-stable/SConstruct#L269

The "+=" used, because OPTIONS_DEFINE=EXAMPLES defined in devel/godot-tools port.

(In reply to comment #68)
> Moreover, the demos files installed into ${DATADIR}/demos, not ${EXAMPLESDIR}.
This was wrong assumption after checking of the attachment #173905 [details].
Jan Beich redefined EXAMPLESDIR, which does the same:
> +EXAMPLESDIR=	${DATADIR}/demos
Overall, the proposed changes by Jan Beich has own right to be used. They are ok from different perspective, where some parts have to be used in devel/godot-tools port, for example. The usage of some new options helpers is interesting, but not much different on result from current approach, in my opinion. Some advantage is removal of bsd.port.pre.mk and bsd.port.post.mk includes.

I included some proposed changes by Jan Beich. I hope this is ok.
Most of the changes discussed and approved before.

(In reply to lightside from comment #79)

> The removal of OPTIONS_DEFINE+=DEBUG (and related code) is not ok,
> because there is a target=debug option available, which maybe used
> for tools=no and tools=yes options:

target=release is only available for runtime only build
target=release_debug is needed by tools to allow debugging the game being developed.
target=debug is meant for debugging the godot tools and runtime code.

The debug option isn't needed in the port, ports are meant for the end user installing software to use, for someone using godot to create a game release_debug provides the needed game runtime debugging, for someone helping to debug and develop godot, they can be expected to be using code from HEAD and not the older release code used in ports. For a runtime only install to be used by games installed by other ports, only offering a release build is reasonable.

(In reply to comment #81)
> The debug option isn't needed in the port, ports are meant for the end user
> installing software to use
What you said is reasonable, but not quite true. There are many ports, which use WITH_DEBUG and/or DEBUG option:
https://github.com/freebsd/freebsd-ports/search?q=WITH_DEBUG

The DEBUG option just activates the WITH_DEBUG=yes:
https://svnweb.freebsd.org/ports/head/Mk/bsd.options.mk?view=markup&pathrev=420340#l469
The WITH_DEBUG usage intended to not strip the binary of debugging information:
https://svnweb.freebsd.org/ports/head/Mk/bsd.port.mk?view=markup&pathrev=420461#l1629
The DEBUG option is not default.

The DEBUG option was proposed and approved from comment #22 for attachment #170772 [details].
Please provide a full patch with your changes for devel/godot port, if you still want to remove such option.

Created attachment 173964 [details]
Proposed patch (since 415742 revision) without DEBUG option

(In reply to comment #82)
> Please provide a full patch with your changes for devel/godot port,
> if you still want to remove such option.
Ok, attached my version of it.

Created attachment 173966 [details]
Proposed patch (since 415742 revision) without DEBUG option

Sorted MAKE_ARGS.

Comment 85 Jan Beich 2016-08-23 18:55:05 UTC

Created attachment 173976 [details]
Eliminate .include <bsd.port.pre.mk> line (for attachment 173959 [details])

|make makesum -C devel/godot| removes distfile for godot-demo-projects. It can be worked around by overriding MAKEFLAGS, see my patch.

> DESKTOP_ENTRIES=	"Godot" "${COMMENT}" "${GODOTFILE}" \
> 			"${GODOTFILE}" "Development;IDE;" ""

Maybe add Game; to Categories unless it's a generic IDE.

(In reply to lightside from comment #71)
> Not sure about compiler:c++11-lib, currently:

Before USE_GCC and USES=compiler are merged consider the above an unexpanded version of the following

  .if ${CHOSEN_COMPILER_TYPE} == gcc
  USE_GCC= yes
  .endif

> ... approved method is to do changes in devel/godot port instead,
> which may reduce possible changes for devel/godot-tools port.

In that case tools=yes associated changes can be converted into OPTIONS_SLAVE, see my patch.

Created attachment 173980 [details]
Proposed patch (since 415742 revision) with suggestions from Jan Beich

(In reply to comment #85)
> |make makesum -C devel/godot| removes distfile for godot-demo-projects.
Yes, the distinfo file was generated from directory of devel/godot-tools port:
% make makesum -C devel/godot-tools

(In reply to comment #85)
> It can be worked around by overriding MAKEFLAGS, see my patch.
Ok, proposed.

(In reply to comment #85)
> Maybe add Game; to Categories unless it's a generic IDE.
The Godot (with tools) related to so called "Game integrated development environment", according to Wikipedia:
https://en.wikipedia.org/wiki/Game_integrated_development_environment
But this is not a game to be added to Game category for desktop entry, I think. Possible to run games (or other applications) with using godot-tools project manager, of course.

(In reply to comment #85)
> Before USE_GCC and USES=compiler are merged consider the above an unexpanded version of the following
> 
> .if ${CHOSEN_COMPILER_TYPE} == gcc
> 	USE_GCC= yes
> .endif
>> ... approved method is to do changes in devel/godot port instead,
>> which may reduce possible changes for devel/godot-tools port.
> 
> In that case tools=yes associated changes can be converted into OPTIONS_SLAVE, see my patch.
Ok, proposed.

I excluded DEBUG option, because of attachment #173966 [details] approval without DEBUG option.
Also I paraphrased and moved some comments.

Added maintainer-approval request for this variant.

Created attachment 173981 [details]
The devel/godot-tools port in shar format with suggestions from Jan Beich

Also added "integrated" word to COMMENT of devel/godot-tools port:
-8<--
COMMENT=	Game integrated development environment
-->8-

Created attachment 173982 [details]
Proposed patch (since 415742 revision, second variant) with suggestions from Jan Beich

(In reply to comment #85)
> Maybe add Game; to Categories unless it's a generic IDE.
Added this variant also, just in case.

Created attachment 173984 [details]
The devel/godot-tools port in shar format with suggestions from Jan Beich

Sorted OPTIONS_* variables.
Replaced blank space with tab for value of OPTIONS_EXCLUDE variable.

Comment 90 Jan Beich 2016-08-24 00:39:24 UTC

(In reply to comment #88)
>> Maybe add Game; to Categories unless it's a generic IDE.
> Added this variant also, just in case.

For maintainer, if omitted Categories in .desktop file are constructed from CATEGORIES in Makefile.

  $ make desktop-categories -C devel/godot
  Development;Game;

Categories are defined by Menu spec which says

  If multiple Main Categories are included in a single desktop entry
  file, the entry may appear more than once in the menu.

https://standards.freedesktop.org/menu-spec/latest/apa.html

It doesn't elaborate on the order, so "Development;Game;IDE;" and "Development;IDE;Game;" maybe interchangeable.

Created attachment 173985 [details]
Proposed patch (since 415742 revision, third variant) with suggestions from Jan Beich

(In reply to comment #88)
> It doesn't elaborate on the order, so "Development;Game;IDE;" and
> "Development;IDE;Game;" maybe interchangeable.
The "Development;Game;IDE;" is more right (main categories first (Development, Game) and additional categories (IDE) second), in my opinion. Fixed.

The developer(s) of Blender Game Engine (which is also listed on Wikipedia (for GIDE)) doesn't add "Game;" to Categories of blender.desktop file, for reference:
https://developer.blender.org/diffusion/B/browse/master/release/freedesktop/blender.desktop;v2.77a

Comment on attachment 173982 [details]
Proposed patch (since 415742 revision, second variant) with suggestions from Jan Beich

(In reply to comment #89)
> The "Development;Game;IDE;" is more right (main categories first
> (Development, Game) and additional > categories (IDE) second), in my opinion.
The "Development;IDE;Game;" is also may be right, according to following page:
https://standards.freedesktop.org/menu-spec/latest/apas02.html

I included game in the port category as godot provides a runtime for other ports, by itself it is only an IDE, before this patch devel/godot was installed once with an option to disable the tools so that only the runtime was installed. The addition of godot-tools adds a clearer separation between installing only the runtime for other ports to depend on and the IDE. Adding it to the users menu of games isn't really appropriate.

Technically the examples provide some "games" but they are only the simplest code that shows how to use a feature or technique you may want to use in your game, the couple of examples that provide some sort of basic gameplay are not something you would play for more than a couple of minutes.

Created attachment 174021 [details]
Proposed patch (since 415742 revision) with suggestions from Jan Beich

Returned TOOLS_DESC.

Created attachment 174023 [details]
Proposed patch (since 415742 revision) with suggestions from Jan Beich

Returned indentation for OPTIONS_DEFINE, which was ok, to exclude this (unnecessary) change.

Comment 96 Kurt Jaeger 2016-08-24 18:51:47 UTC

95 replies -- I guess we have a patch now. Testbuilds@work

Comment 97 commit-hook 2016-08-24 19:22:14 UTC

A commit references this bug:

Author: pi
Date: Wed Aug 24 19:21:12 UTC 2016
New revision: 420818
URL: https://svnweb.freebsd.org/changeset/ports/420818

Log:
  devel/godot: Update to 2.1 and add minimal devel/godot-tools slave port

  - Update to 2.1 release
  - Add LICENSE_FILE for MIT license
  - Add CCBYv3 license for logo
  - Add devel/xdg-user-dirs, devel/xdg-utils and x11/xmessage to RUN_DEPENDS
  - Add xrandr to USE_XORG
  - Replace USE_OPENSSL=yes with USES+=ssl
  - Add gl to USE_GL
  - Remove GLU dependency - glu.h was included from imported glew.h
    but GLU components are not used
  - Add DEBUG option and clarify build targets
  - Replace TOOLS option with devel/godot-tools port, while devel/godot
    port used as runtime
  - Allow to build ALSA and PulseAudio drivers independently
  - Allow to build RtAudio driver with OSS or ALSA or PulseAudio
  - Set OSS option by default
  - Add desktop entry for godot-tools
  - Add sed patch to fix executable paths inside of
    ${WRKSRC}/platform/x11/os_x11.cpp file
  - Remove mkdir, because of COPYTREE_SHARE usage, which recreates
    destination directories
  - Remove RM command for demos/2d/hexamap/.fscache file, because the
    file is not available for current version
  - Add patch to enable OSS usage for RtAudio driver on FreeBSD
  - Add patch to fix issue(s) with OS_Unix::execute and
    OS_Unix::get_executable_path functions on FreeBSD
  - Add patch to fix layout of --help text
  - Remove pkg-plist and use dynamic package list with PORTDATA and PLIST_FILES
  - Use current head commit of 2.1 branch from new github repo
    godotengine/godot-demo-projects to install examples (this was
    previously part of godot repo)

  PR:		209742
  Submitted by:	lightside@gmx.com
  Approved by:	FreeBSD@Shaneware.biz

Changes:
  head/devel/Makefile
  head/devel/godot/Makefile
  head/devel/godot/distinfo
  head/devel/godot/files/patch-SConstruct
  head/devel/godot/files/patch-drivers_rtaudio_RtAudio.cpp
  head/devel/godot/files/patch-drivers_rtaudio_RtAudio.h
  head/devel/godot/files/patch-drivers_rtaudio_audio__driver__rtaudio.cpp
  head/devel/godot/files/patch-drivers_unix_os__unix.cpp
  head/devel/godot/files/patch-main_main.cpp
  head/devel/godot/files/patch-platform_x11_detect.py
  head/devel/godot/pkg-plist
  head/devel/godot-tools/
  head/devel/godot-tools/Makefile

Comment 98 Kurt Jaeger 2016-08-24 19:22:19 UTC

Committed, thanks!

(In reply to comment #7)
> I would suggest you consider submitting a patch to update
> Mk/bsd.license.db.mk with the CC variations, maybe with a patch that also
> updates the various ports that currently have their own CC info.
Just for note:
Dmitry Marakasov (amdmi3@) proposed this in review D7852 and committed in ports r421995.
The devel/godot port was updated in ports r422001.