Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update sourceList.txt files to match capitalization of NVEL files. #44

Open
d-diaz opened this issue May 20, 2024 · 10 comments
Open

Update sourceList.txt files to match capitalization of NVEL files. #44

d-diaz opened this issue May 20, 2024 · 10 comments

Comments

@d-diaz
Copy link

d-diaz commented May 20, 2024

Working on Linux or MacOSX to build FVS from source, we can pull the source code including the NVEL submodule now using
git clone --recurse-submodules https://github.com/USDAForestService/ForestVegetationSimulator.git and retrieve the NVEL code that was recently moved into the ForestVegetationSimulator/volume/NVEL folder as a submodule. However, many of the files within the NVEL repo are named with a mix of capitalizations while the FVS*_sourceList.txt files specify all these filenames to completely lower-cased.

Recommend updating the FVS*_sourceList.txt files to use the capitalization matching incoming NVEL repo rather then requiring the user to change all the filenames to lowercase to use existing sourceLists or to update sourceLists themselves.

Here is what currently happens (building using Windows Subsystem for Linux):

>> git clone --recurse-submodules https://github.com/USDAForestService/ForestVegetationSimulator.git
>> cd ForestVegetationSimulator/bin
>> make FVSpn
OS: ; OSTYPE: ; WIN: -Dunix  ; OSARCH: l64; SLIBSUFX: .so
PIC: -fPIC; PRGSUFX:
mingw64: ; FCprf: ; CCprf:
FVSprgs: FVSak FVSbc FVSbm FVSca FVSci FVScr FVScs FVSec FVSem FVSie FVSkt FVSls FVSnc FVSne FVSoc FVSon FVSop FVSpn FVSsn FVSso FVStt FVSut FVSwc FVSws
CANprgs: FVSbc FVSon
USprgs: FVSak FVSbm FVSca FVSci FVScr FVScs FVSec FVSem FVSie FVSkt FVSls FVSnc FVSne FVSoc FVSop FVSpn FVSsn FVSso FVStt FVSut FVSwc FVSws
make: *** No rule to make target '../volume/NVEL/beqinfo.inc', needed by 'FVSpn'.  Stop.

When all the files in the NVEL folder are renamed to be lower case, FVS builds successfully.

@ghost
Copy link

ghost commented May 20, 2024 via email

@d-diaz
Copy link
Author

d-diaz commented May 21, 2024

After trying the renaming of a NVEL files within a single sourceList.txt file, I can confirm @nickcrookston was right, and the build fails with the same error, first triggered by BEQINFO.inc

Agree that fixing this upstream would be ideal. That being said, I'd trust y'all's better judgement whether any of you could submit a pull request to NVEL to get that moving or if NVEL would accept a pull request from someone like me. Perhaps in the meantime y'all could make a fork of the NVEL library, update the file names there, and then have that version of the NVEL equations employed in this repo instead of using the main NVEL repo as a submodule? For example, maybe you could update the FMSC-Measurements/VolumeLibrary repo?

@wagnerds
Copy link
Collaborator

wagnerds commented May 21, 2024

I'll have another discussion with NVEL. Last time I brought it up, they do not accept pull requests and prefer to do things via email.

@jgrn307
Copy link

jgrn307 commented May 30, 2024

Is there a suggested "hack" fix in the meantime? We have an automatic build system and this broke our workflow.

@jgrn307
Copy link

jgrn307 commented May 30, 2024

Update: what's even weirder is I've been rolling back the SHA I'm using to try to build off an earlier version of this (we were able to do the build a few months back) and I can't get it to build no matter what SHA I use (e.g. I'm back to using January 2024 commit but that's giving me the same error). Is this connected to a different github repo or something for those files? I don't understand why rolling back my pull isn't working to get this functional again.

@wagnerds
Copy link
Collaborator

@jgrn307 What was the source of the SHA you are reverting to? Was is just based off the date listed in this repository or from a source you previously had working? I suspect the SHA you may be referencing from January was the state of our code base in our Enterprise repository from that date, where the submodule repository change was already in effect. We held off on releasing that update until now as we only have Windows machines to test on.

Are you able to redirect your build to a previous tagged release? Maybe https://github.com/USDAForestService/ForestVegetationSimulator/tree/FS2023.3 ?

Also, what is your build environment? Some version of Linux I assume?

@wagnerds
Copy link
Collaborator

@jgrn307 @d-diaz

I may have misunderstood a previous comment. @d-diaz You said it built successfully when you changed the NVEL files to lowercase. I had posted an R script that should help users convert their local copy of the NVEL files to all lowercase, and may have misunderstood the response.

You said you renamed the files in the FVSxx_SourceList.txt to match the incoming NVEL files, and that didn't work as the file names did not match the NVEL 'INCLUDE' statements, which makes sense, but did you try the script I included that would convert the NVEL files to lowercase to match the 'INCLUDE' statements as well as the sourceList.txt file names?

Can you help me confirm whether running this script in R changes the file names within the volume/NVEL/ directory to lowercase within you local copy of NVEL?
setwd("PathToRepository/ForestVegetationSimulator/")
fn = dir('./volume/NVEL/')
fn = paste0('./volume/NVEL/',fn)
file.rename(fn, tolower(fn))

This seems to work in my instance of WSL, so I am looking for a bit of feedback on if this is a viable workaround.

@jgrn307
Copy link

jgrn307 commented May 30, 2024

I was able to get the build working by reverting to 3b09ae2 without the aforementioned error. I'm doing a linux-based Docker build of FVS.

I'll try the other tag tomorrow when I get back to this!

--j

@jgrn307 What was the source of the SHA you are reverting to? Was is just based off the date listed in this repository or from a source you previously had working? I suspect the SHA you may be referencing from January was the state of our code base in our Enterprise repository from that date, where the submodule repository change was already in effect. We held off on releasing that update until now as we only have Windows machines to test on.

Are you able to redirect your build to a previous tagged release? Maybe https://github.com/USDAForestService/ForestVegetationSimulator/tree/FS2023.3 ?

Also, what is your build environment? Some version of Linux I assume?

@d-diaz
Copy link
Author

d-diaz commented Jun 6, 2024

@jgrn307 @d-diaz

I may have misunderstood a previous comment. @d-diaz You said it built successfully when you changed the NVEL files to lowercase. I had posted an R script that should help users convert their local copy of the NVEL files to all lowercase, and may have misunderstood the response.

You said you renamed the files in the FVSxx_SourceList.txt to match the incoming NVEL files, and that didn't work as the file names did not match the NVEL 'INCLUDE' statements, which makes sense, but did you try the script I included that would convert the NVEL files to lowercase to match the 'INCLUDE' statements as well as the sourceList.txt file names?

Can you help me confirm whether running this script in R changes the file names within the volume/NVEL/ directory to lowercase within you local copy of NVEL? setwd("PathToRepository/ForestVegetationSimulator/") fn = dir('./volume/NVEL/') fn = paste0('./volume/NVEL/',fn) file.rename(fn, tolower(fn))

This seems to work in my instance of WSL, so I am looking for a bit of feedback on if this is a viable workaround.

I implemented a lower casing step in a shell script and was able to resume builds for US modules. Because the source lists for Canadian variants haven’t been updated to point to new NVEL paths, they are still failing:

cd ~/projects 
git clone --recurse-submodules https://github.com/USDAForestService/ForestVegetationSimulator.git
cd ~/projects/ForestVegetationSimulator/volume/NVEL
for f in `find .`; do mv -v "$f" "`echo $f | tr '[A-Z]' '[a-z]'`"; done
cd ~/projects/ForestVegetationSimulator/bin
make US -j12
sudo find ~/projects/ForestVegetationSimulator/bin/FVS* -maxdepth 0 -type f ! -name "*_sourceList.txt" -exec cp -t /usr/local/bin {} +
make clean

This is a duct-tape fix. Ideally FVS source code will be provided in a way that doesn’t require these kinds of patches by the user to build.

Even if y’all are working on windows, would encourage using WSL or Docker to run build tests. As I mentioned above, to fix this without waiting for NVEL, you could incorporate a different repo as the submodule, such as a fork of the NVEL repo where y’all have already lower-cased the file names.

@d-diaz
Copy link
Author

d-diaz commented Oct 3, 2024

Any update on potential upstream fixes to build error due to capitalizations within volume/NVEL? If NVEL isn't likely to address these capitalization issues at source, an FVS fork of the NVEL repo with capitalization fixed so that you could incorporate the forked version as a submodule would be a relatively lightweight fix.

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

No branches or pull requests

3 participants