Skeldal a autoconf (diskuse)

[Jardík]

Navrhuji řešení použít environment proměnné XDG_DATA_HOME a XDG_DATA_DIRS (s příslušnými fallbacky dle freedesktop "standardu") a pokud nikde data nejsou, tak teprve poté přistoupit k použití hardkódované cesty nastavené configure skriptem. Takové řešení je (podle mě) nejlepší.

[Martin Doucha]

Nikdo ti nebrání poslat patch. Jestli s tím budou lidé souhlasit a ten kód bude vypadat rozumně, tak nevidím důvod, proč to nezařadit.

[Jardík]

Já ten program nepoužívám, ani nevím, k čemu to je. Já jen napsal, jak by to bylo podle mě nejlepší. Jestli to tak někdo chce/nechce, nebo jestli to tak někdo udělá/neudělá, mi je fakt jedno. Věta "Nikdo ti nebrání poslat patch" mi vyzní jako "trhni si nohou a nechtěj mi přidělávat starosti". Teď jsem kouknul do jednoho náhodného zdrojáku, spatřil myšlenku sizeof() vrací int a tak si o tomto projektu dále myslím své a mlčím

[polymorf]

+1 tiež ma jeho odpoveď trochu šokovala, myslel som že sa pýta ako na to.

[vain]

No evidentně ptal.

rád si nechám v diskuzi poradit

[Martin Doucha]

Co je šokujícího na tom, že na feature request, který nemá nijak zásadní vliv na funkčnost hry, odpovím "tak to napiš sám a pošli mi patch"? Ze 144 commitů v mojí větvi jsou jen tři patche od někoho jiného, z toho jeden jsou ikonky a další jsem musel sám z poloviny přepsat, protože autor se mi na žádost o vylepšení patche už neozval.

BTW, měl bych Jardíkovi poděkovat. Při opravování typů proměnných a návratových hodnot, co přímo pracují se sizeof, se mi podařilo najít a opravit jeden bug v pointerové aritmetice.

[polymorf]

Jak som pochopil tento blog:

Ty: Program skeldal, nieco zmenili a neviem ako upravit konfigurak aby nacitaval data z ineho miesta. Neviete niekto ako na to?

Jardik: Myslim ze by stacilo upravit premennu X a Y.

Ty: Nikdo ti nebrání poslat patch.

[Martin Doucha]

Tak to jsi pochopil špatně. Tuhle změnu ve Skeldalu jsem spáchal já, a tohle je v podstatě changelog, aby se uživatelé nedivili, že jim Skeldal najednou odmítá načítat data. Dotaz byl na znalce Autoconfu co napsat do configure.ac, aby build systém defaultně hledal data na starém místě i bez zásahu uživatele.

[polymorf]

Aj tak znies ako buran:

Ty: Upravil som cestu dat v programe skeldal, neviem ako upravit configure.ac aby nacitaval data zo spravneho miesta. Neviete niekto ako na to?

Jardik: Myslim ze by stacilo upravit premennu X a Y.

Ty: Nikdo ti nebrání poslat patch.

[Martin Doucha]

V tom případě jsi nepochopil ani Jardíkův komentář. XDG_* tenhle problém nevyřeší, protože na různých strojích můžou mít různý obsah. Jardík jenom prudí s dalším Jediným® Správným© Řešením™, navíc na úplně jiný problém.

[Jardík]

Tedy jsem ti opravil fci (idx by si zasloužilo být size_t, ale ...:

// skeldal/branches/next_ghost/unix/init.cpp

void Sys_SetPath(unsigned idx, const char *path) {
	size_t len = strlen(path); // tady byl int
	assert(idx < PATHTABLE_SIZE);

	if (pathtable[idx]) {
		free(pathtable[idx]);
	}

	// tady nebyl test *path == 0, což mohlo způsobit pád v kódu
	// níže, protože len-1 by šlo na neplatný index SIZE_MAX
	if (!*path || *path == 0) { 
		pathtable[idx] = (char*)malloc(1);
		pathtable[idx][0] = '\0';
		return;
	}

	if (path[len - 1] != '/') {
		len++;
	}

	pathtable[idx] = (char*)malloc(len + 1);
	strcpy(pathtable[idx], path);
	pathtable[idx][len-1] = '/';
	pathtable[idx][len] = '\0';
}

Ještě tam teda chybí kontrola návratových hodnot z malloc(), to už se mi nechce dělat.

Další chyby:

// není reentrant
char *Sys_FullPath(unsigned idx, const char *file) {
	static char ret[PATH_MAX];
	assert(idx < PATHTABLE_SIZE);
	// tady může dojít k přetečení fixně velikého bufferu
	strcpy(ret, pathtable[idx]);
	strncat(ret, file, PATH_MAX - strlen(ret) - 1);
	return ret;
}

// není reentrant
char *Sys_DOSPath(unsigned defdir, const char *path) {
	static char ret[PATH_MAX];
	int i, length;        // "má" být size_t
	assert(defdir < PATHTABLE_SIZE);

	// možnost přetečení bufferu
	strcpy(ret, pathtable[defdir]);
	length = strlen(ret);
	strncat(ret + length, path, PATH_MAX - length - 1);
	strupr(ret + length);

	for (i = length; ret[i]; i++) {
		if (ret[i] == '\\') {
			ret[i] = '/';
		}
	}

	return ret;
}

void Sys_Init(void) {
	char home[PATH_MAX]; // smyšlená hodnota maximální velikosti cesty
	int len; // size_t
	// možnost přetečení bufferu, getenv() může vrátit NULL
	strcpy(home, getenv("HOME"));
	len = strlen(home);
	// neplatný index při len == 0
	if (home[len-1] != '/') {
		home[len++] = '/';
	}
	// další možnost přetečení bufferu
	strcpy(home + len, ".skeldal/");

Nakonec dotaz: znáš třeba std::map a std::string? Když je to C++ kód, proč to nepoužít? Ušetřil bys si spoustu starostí, např. Sys_SetPath by mohl vypadat nějak takto:

typedef std::map<unsigned, std::string> Mapa;
Mapa mapa;

void Sys_SetPath(unsigned id, const std::string& path)
{
  std::string& oldVal = mapa[id];
  std::string::size_type len;
  oldVal = path;
  len = oldVal.size();
  if (len > 0 && oldVal[len-1] != '/') oldVal += '/';
}

A při chybné alokaci ti to prostě vyprskne std::bad_alloc

[Jardík]

Příloha:

• a.log (94102 bytů)

BTW máš co dělat, tady je log z kompilace (prsknul jsem tam -Wall -pedantic):

[Martin Doucha]

Hezké. Jenže většina těch problémů je v původním kódu z roku 1997. Opravovat to budu průběžně až na daný kus kódu přijde řada.

[Martin Doucha]

Zformátuj to a pošli to jako patch, ať jsou vidět změny. STL znám a odmítám to použít, protože bych tím akorát zablokoval možnost portovat Skeldal na mobilní platformy s omezenou pamětí.

[Jardík]

STL znám a odmítám to použít, protože bych tím akorát zablokoval možnost portovat Skeldal na mobilní platformy s omezenou pamětí.

To je škoda. Já si trochu procházel ten kód a místama jsou tam takové čuňárny a je to tak nepřehledné, všude možně chybí kontrola chyb, některé funkce vůbec neupozorní na chyby (ani nějakým kódem, ani výjimkou, ani vypsáním) a vypadá to fakt neudržitelně. Ale snaha se cení.

[Vin]

A STL by těm čuňárnám nějak zamezilo?

Jinak sám autor napsal, že by to dnes řešil jinak - http://www.root.cz/clanky/rozhovor-s-ondrejem-novakem-vyvojarem-hry-brany-skeldalu/

otázkou je, jestli by se tehdy stihnul deadline pro vydání hry, než by se to naučil (navíc, jak zmiňuje v rozhovoru, potřeboval řešit jiné problémy, které dnes už asi nejsou).

[Bedňa]

Mirek Fidler o STL:

Proč děláme takovou hloupost? Stručná, byť možná trochu nesrozumitelná, odpověď na tuto otázku by se dala shrnout do věty: "Protože STL vás nutí si přát, aby C++ mělo automatickou správu paměti (garbage collector - GC)".

Problém je v tom, že do STL kontejneru můžete vkládat pouze objekty, které mají definovanou kopii. Navíc, pokud aspoň trochu dbáte na rychlost aplikace, musí tato kopie pokud možno být jednoduše proveditelná (rychlá).

To v praxi znamená, že do STL kontejneru je možné efektivně ukládat fundamentální typy, možná ještě tak řetězce a pár dalších jednoduchých typů. Všechno ostatní lze uložit pouze nepřímo pomocí ukazatele. Tím se ztrácí jedna ze základních výhod, kterou používaní kontejnerových šablon přináší - automatická správa paměti.

[Jardík]

V C++0x to řeší move constructor.

[Bedňa]

Hackery sa takto bavia bežne, keď nevieš o čom je reč tak sa nezapájaj, sa čudujem že Martin na teba vôbec reagoval.

[stativ]

Řekl bych, že vzhledem k tomu, z jakých zdrojáků to vzešlo je to detail a cením si práce, které na tom Martin udělal, aby to vůbec fungovalo s moderními systémy.

[Drew]

Hm, to by se ale týkalo ledatak uložených pozic a nastavení (je ve skeldalu vůbec nějaké?) - herní data by přece měla být nainstalována ne v domovském adresáři nějakého uživatele ale někde, kde jsou dostupná všem, ne?

[Martin Doucha]

XDG_DATA_DIRS má na mém stroji hodnotu /usr/share:/usr/local/share:/usr/share a XDG_DATA_HOME nemám nastavenou vůbec. Tyhle proměnné jsou dobré tak akorát pro proprietární soft, aby se dal instalovat na víc možných míst. Skeldal je open source a od začátku mám v plánu protlačit do repozitářů minimálně Gentoo a Debianu, takže takovou věc nepotřebuje.

[Scarabeus IV]

A ty beres jaky drogy chlape, i kdyz se Jardik vetsinou chova jako idiot tak ma pravdu.

XDG je freedesktop specifikace...

A nyni par veci k autotools:
1) v repozitari se ponechavaji pouze Makefile.am a configure.{ac,in} zbytek je generovan pouze pro release.
2) proc nepouzivas pkgconfig kdyz hledas sdl a sdl-mixer: PKG_CHECK_MODULES(SDL, sdl sdl-mixer) a pak se pouzije v Makefile.am skeldal_LDADD = $(SDL_LIBS) skeldal_CXXFLAGS=$(SDL_CFLAGS) nebo tak nejak. 3) zadratovat cestu v prubehu configure je sakrapekna prasarna viz XDG specifikace vejs, ale pokud po tom touzis tak se vyplati tam dat neco jako --with-gamedata=/path do configure, a to predat do Makefile.am nez redefinovat datadir.
4) CPPFLAGS jsou nastaveni pro C Pre Processor ne pro C++ to je CXXFLAGS.
5) Chybi tam autogen.sh na generovani configure/Makefile, viz bod 1.

ad 3)

AC_ARG_WITH(gamedata-prefix,
	[AS_HELP_STRING([--with-gamedata-prefix=PREFIX], [Prefix for game data.])],
)

ad 5)

set -e
: ${AUTORECONF:=autoreconf}
if test "$*"; then
	ARGS="$*"
else
	if test -f config.log ; then
		ARGS=`grep '^  \$ \./configure ' config.log | sed 's/^  \$ \.\/configure //' 2> /dev/null`
	fi
fi

echo "Running ${AUTORECONF}..."
$AUTORECONF --install --verbose
test x$NOCONFIGURE = x && echo "Running ./configure $ARGS" && ./configure $ARGS

[Martin Doucha]

ad 1) a 5) Neznám moc balíků, které by tohle dělaly, a UNIXový standard je ./configure; make; make install. Dokud se ze "svaté trojice" nestane čtveřice, která začíná ./autogen.sh nebo něčím podobným, budu mít configure skript přímo v repozitáři, stejně jako většina ostatních projektů. Navíc takový skript GNU Coding Standards nevyžaduje.

ad 2) Díky za tip, tohle jsem neznal.

ad 3) Podle GNU Coding Standard je to zcela v pořádku a já se s dovolením budu držet GNU Coding Standards. Freedesktop si může vymýšlet, co chce, ale to ještě neznamená, že to má smysl používat.

ad 4) -Dcosi je flag preprocesoru, takže AM_CPPFLAGS je naprosto správné umístění. CPPFLAGS se používá při kompilaci C i C++, CXXFLAGS jen pro C++.

[Scarabeus IV]

1 az 5) Podivej se na cgit.freedesktop.org nebo http://git.gnome.org/browse/.

3) Prekvapive tahle prkotina kterou oznacujes za vymyslenost kterou nema smysl pouzivat je pouzivana vsemy DE aprotoze se dodrzuje na vsech platformach stejne. Zaroven uz to nastesti zacina podporovat spousta 3rd party aplikaci taky takze treba v home slozkach prestane bejt tak nechutnej bordel.

4) no ja se ani nekoukal co v ni je, vetsinou je pouzita blbe, v tomhle pripade bych ovsem spis sel do skeldal_CPPFLAGS nez to nastavovat globalne :)

[Martin Doucha]

ad 3) DE to implementovat musí, jinak celá tahle ptákovina nebude fungovat. Já ale portuju hru, ne DE.