support src-one target and backlevel icus #13

srl295 · 2019-04-10T00:44:17Z

added some extra .gitignores for other branches

Fixes: #12

- added some extra .gitignores for other branches Fixes: #12

roubert · 2019-04-10T12:37:38Z

Makefile

 DISTROS_SMALL=ubuntu fedora
 SRC=src/icu
-REV:=$(shell (cd $(SRC) && (git describe --tags --exact-match 2>/dev/null || git rev-parse --short HEAD)) || echo 'unknown')
+REV:=$(shell (cd $(SRC) >/dev/null 2>/dev/null && (git describe --tags --exact-match 2>/dev/null || git rev-parse --short HEAD)) || echo 'unknown')


What's the purpose of the first >/dev/null here, under what circumstances would cd ever output anything to stdout?

not sure which circumstances. but it is certainly doing so. might be sh vs bash

I'd suggest that you remove the first >/dev/null and in case you later actually encounter a case where it's needed you can then add it with an explanation of the circumstances under which it's needed.

It was needed on my mac, otherwise I get REV=/some/path 64.2. I will test again.

mystery solved: complete user error, https://superuser.com/a/90537/13787

In that case, I think that unset CDPATH, as recommended there, seems like the most straightforward solution, which leaves no doubt as to what the purpose of it is.

In that case, I think that unset CDPATH, as recommended there, seems like the most straightforward solution

I'm inclined to remove >/dev/null 2>/dev/null everywhere and (possibly) just document to not set CDPATH.

Huh, i found something! See https://bosker.wordpress.com/2012/02/12/bash-scripters-beware-of-the-cdpath/ (kind of a public service announcement):

You can avoid it in some cases by using cd ./foo, which does not consult CDPATH. But this is not a panacea: it can’t easily be used with paths that might be absolute or relative, such as dirname "$0", so I think unsetting CDPATH is still the best way to deal with it.

That actually implies that Q=$(cd ./stuff; echo bar) or P�=$(cd /path/to/stuff; echo baz) might be best - using a non-ambiguous path starting with / or ./.

So setting:

SRC=./src/icu

…at the top of this Makefile actually avoids the CDPATH issue.

roubert · 2019-04-10T12:42:11Z

src/bin/check-icu4c-version.sh

+    shift
+fi
+
+if [ ! -f  ./source/common/unicode/uversion.h ];


The initial ./ doesn't add any value here, the current directory is always the base for any relative path.

roubert · 2019-04-10T12:42:33Z

src/bin/check-icu4c-version.sh

+    echo "unknown-no-uversion"
+    exit 1
+fi
+( grep -h "^#define U_ICU_VERSION " ./source/common/unicode/uvernum.h || echo unknown-no-define) | cut -d' ' -f3 | tr -d '"'


Initial ./.

roubert · 2019-04-10T12:44:07Z

src/bin/check-icu4c-version.sh

+    echo "unknown-no-uversion"
+    exit 1
+fi
+( grep -h "^#define U_ICU_VERSION " ./source/common/unicode/uvernum.h || echo unknown-no-define) | cut -d' ' -f3 | tr -d '"'


The -h option is the default when grep only has one file to search, it's redundant here.

I originally had included source/common/unicode/uversion.h, however, it was not needed. (it predates 4.8)

roubert · 2019-04-10T12:47:45Z

src/bin/dist_icu64.mk

@@ -0,0 +1,99 @@
+# Copyright (C) 2016 and later: Unicode, Inc. and others.


Copyright 2016? Is this file a copy of some existing file? If it is, would it be possible to make git show that in the change history?

The way it is now, it looks as if your wrote a new file from scratch, but put the wrong date on it.

yes, it's a copy of dist.mk from ICU trunk. I'll fix the date.

yes, it's a copy of dist.mk from ICU trunk.

Ah, from a file in a different repository? That explains why the diff here shows it as new ... I think it could be valuable if you for those files that are copies listed exact paths of the files they're copies of. That'll make it much easier to track down where things came from if that turns out to be needed in the future.

I'll fix the date.

It looks as if you forgot to actually do that.

It looks as if you forgot to actually do that.

No, as I said above, I wanted finish using this script (getting the maint tarballs out the door) before coming back to this PR.

roubert · 2019-04-10T12:49:17Z

src/bin/makesrc.sh

@@ -0,0 +1,46 @@
+#!/bin/sh
+# © 2016 and later: Unicode, Inc. and others.


Also this file, is it a copy of some existing file?

it's a copy of one of the sibling scripts in the same directory. Will fix.

roubert · 2019-04-10T12:50:49Z

src/bin/makesrc.sh

+# #FN=icu-r$(svnversion /src/icu/ | tr -d ' ')-$(bash /src/icu/icu4c/source/config.guess)-${1:RANDOM}
+# tar cfpz /dist/${FN}-sdoc.tgz ./doc || exit 1
+#cd /dist
+#md5sum ${FN}.tgz | tee ${FN}.tgz.md5


I don't think there should be any commented-out code in the file (at least not without a comment explaining under what circumstances someone using this should uncomment and use that code).

So, please either remove the commented-out code or add a comment explaining when a user should be doing that.

Yes, i was waiting until the maint branches were shipped to finish this. I have a commit that removes this code.

- also add a workaround for icu 58/59 Fixes: #12

- you might want to disable this if you want to look at the container later

roubert · 2019-04-15T19:08:30Z

Makefile

 DISTROS_SMALL=ubuntu fedora
 SRC=src/icu
-REV:=$(shell (cd $(SRC) && (git describe --tags --exact-match 2>/dev/null || git rev-parse --short HEAD)) || echo 'unknown')
+REV:=$(shell (cd $(SRC) >/dev/null 2>/dev/null && (git describe --tags --exact-match 2>/dev/null || git rev-parse --short HEAD)) || echo 'unknown')


I'd suggest that you remove the first >/dev/null and in case you later actually encounter a case where it's needed you can then add it with an explanation of the circumstances under which it's needed.

roubert · 2019-04-15T19:08:51Z

src/bin/check-icu4c-version.sh

@@ -0,0 +1,17 @@
+#!/bin/sh
+# © 2016 and later: Unicode, Inc. and others.


roubert · 2019-04-15T19:18:50Z

src/bin/dist_icu64.mk

@@ -0,0 +1,99 @@
+# Copyright (C) 2016 and later: Unicode, Inc. and others.


yes, it's a copy of dist.mk from ICU trunk.

Ah, from a file in a different repository? That explains why the diff here shows it as new ... I think it could be valuable if you for those files that are copies listed exact paths of the files they're copies of. That'll make it much easier to track down where things came from if that turns out to be needed in the future.

I'll fix the date.

It looks as if you forgot to actually do that.

roubert · 2019-04-15T19:23:38Z

src/bin/makesrc.sh

+# #FN=icu-r$(svnversion /src/icu/ | tr -d ' ')-$(bash /src/icu/icu4c/source/config.guess)-${1:RANDOM}
+# tar cfpz /dist/${FN}-sdoc.tgz ./doc || exit 1
+#cd /dist
+#md5sum ${FN}.tgz | tee ${FN}.tgz.md5


So, please either remove the commented-out code or add a comment explaining when a user should be doing that.

support src-one target and backlevel icus

4a7d279

- added some extra .gitignores for other branches Fixes: #12

srl295 requested review from roubert and yumaoka April 10, 2019 00:44

srl295 self-assigned this Apr 10, 2019

roubert requested changes Apr 10, 2019

View reviewed changes

srl295 added 2 commits April 11, 2019 12:17

update for review comment

1f31b84

- also add a workaround for icu 58/59 Fixes: #12

version

399a118

srl295 requested a review from roubert April 11, 2019 19:19

use run --rm so that containers don't dangle

07772d4

- you might want to disable this if you want to look at the container later

roubert requested changes Apr 15, 2019

View reviewed changes

srl295 added 2 commits July 19, 2019 10:07

temp hack to build backlevel

2563efd

local hacks

6bab9c7

Base automatically changed from master to main March 24, 2021 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support src-one target and backlevel icus #13

support src-one target and backlevel icus #13

srl295 commented Apr 10, 2019

roubert Apr 10, 2019

srl295 Apr 10, 2019

roubert Apr 15, 2019

srl295 Apr 15, 2019

srl295 Sep 10, 2019

roubert Sep 10, 2019

srl295 Sep 10, 2019

roubert Apr 10, 2019

roubert Apr 10, 2019

roubert Apr 10, 2019

srl295 Apr 10, 2019

roubert Apr 10, 2019

srl295 Apr 10, 2019

roubert Apr 15, 2019

srl295 Apr 15, 2019

roubert Apr 10, 2019

srl295 Apr 10, 2019

roubert Apr 10, 2019

roubert Apr 15, 2019

srl295 Apr 15, 2019

roubert Apr 15, 2019

roubert Apr 15, 2019

roubert Apr 15, 2019

roubert Apr 15, 2019

		@@ -0,0 +1,99 @@
		# Copyright (C) 2016 and later: Unicode, Inc. and others.

		@@ -0,0 +1,46 @@
		#!/bin/sh
		# © 2016 and later: Unicode, Inc. and others.

		@@ -0,0 +1,17 @@
		#!/bin/sh
		# © 2016 and later: Unicode, Inc. and others.

support src-one target and backlevel icus #13

Are you sure you want to change the base?

support src-one target and backlevel icus #13

Conversation

srl295 commented Apr 10, 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

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

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